* [PATCH v9 04/10] drm/mediatek: add BLS component
From: YT Shen @ 2016-11-11 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478865346-19043-1-git-send-email-yt.shen@mediatek.com>
Add BLS component for PWM + GAMMA function
Signed-off-by: YT Shen <yt.shen@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 5 ++++-
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index 661a4a0..b78b2e6 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -235,6 +235,7 @@ static void mtk_gamma_set(struct mtk_ddp_comp *comp,
[MTK_DISP_PWM] = "pwm",
[MTK_DISP_MUTEX] = "mutex",
[MTK_DISP_OD] = "od",
+ [MTK_DISP_BLS] = "bls",
};
struct mtk_ddp_comp_match {
@@ -245,6 +246,7 @@ struct mtk_ddp_comp_match {
static const struct mtk_ddp_comp_match mtk_ddp_matches[DDP_COMPONENT_ID_MAX] = {
[DDP_COMPONENT_AAL] = { MTK_DISP_AAL, 0, &ddp_aal },
+ [DDP_COMPONENT_BLS] = { MTK_DISP_BLS, 0, NULL },
[DDP_COMPONENT_COLOR0] = { MTK_DISP_COLOR, 0, &ddp_color },
[DDP_COMPONENT_COLOR1] = { MTK_DISP_COLOR, 1, &ddp_color },
[DDP_COMPONENT_DPI0] = { MTK_DPI, 0, NULL },
@@ -303,7 +305,8 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
comp->id = comp_id;
comp->funcs = funcs ?: mtk_ddp_matches[comp_id].funcs;
- if (comp_id == DDP_COMPONENT_DPI0 ||
+ if (comp_id == DDP_COMPONENT_BLS ||
+ comp_id == DDP_COMPONENT_DPI0 ||
comp_id == DDP_COMPONENT_DSI0 ||
comp_id == DDP_COMPONENT_PWM0) {
comp->regs = NULL;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index 2f6872a..30faf46 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -36,11 +36,13 @@ enum mtk_ddp_comp_type {
MTK_DISP_PWM,
MTK_DISP_MUTEX,
MTK_DISP_OD,
+ MTK_DISP_BLS,
MTK_DDP_COMP_TYPE_MAX,
};
enum mtk_ddp_comp_id {
DDP_COMPONENT_AAL,
+ DDP_COMPONENT_BLS,
DDP_COMPONENT_COLOR0,
DDP_COMPONENT_COLOR1,
DDP_COMPONENT_DPI0,
--
1.9.1
^ permalink raw reply related
* [PATCH v9 03/10] drm/mediatek: add shadow register support
From: YT Shen @ 2016-11-11 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478865346-19043-1-git-send-email-yt.shen@mediatek.com>
We need to acquire mutex before using the resources,
and need to release it after finished.
So we don't need to write registers in the blanking period.
Signed-off-by: YT Shen <yt.shen@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 76 ++++++++++++++++++++-------------
drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 25 +++++++++++
drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 2 +
drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 +
4 files changed, 75 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
index 01a21dd..a4f2b3a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
@@ -329,6 +329,42 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
pm_runtime_put(drm->dev);
}
+static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
+{
+ struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+ struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
+ struct mtk_ddp_comp *ovl = mtk_crtc->ddp_comp[0];
+ unsigned int i;
+
+ /*
+ * TODO: instead of updating the registers here, we should prepare
+ * working registers in atomic_commit and let the hardware command
+ * queue update module registers on vblank.
+ */
+ if (state->pending_config) {
+ mtk_ddp_comp_config(ovl, state->pending_width,
+ state->pending_height,
+ state->pending_vrefresh, 0);
+
+ state->pending_config = false;
+ }
+
+ if (mtk_crtc->pending_planes) {
+ for (i = 0; i < OVL_LAYER_NR; i++) {
+ struct drm_plane *plane = &mtk_crtc->planes[i];
+ struct mtk_plane_state *plane_state;
+
+ plane_state = to_mtk_plane_state(plane->state);
+
+ if (plane_state->pending.config) {
+ mtk_ddp_comp_layer_config(ovl, i, plane_state);
+ plane_state->pending.config = false;
+ }
+ }
+ mtk_crtc->pending_planes = false;
+ }
+}
+
static void mtk_drm_crtc_enable(struct drm_crtc *crtc)
{
struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
@@ -405,6 +441,7 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
+ struct mtk_drm_private *priv = crtc->dev->dev_private;
unsigned int pending_planes = 0;
int i;
@@ -423,6 +460,13 @@ static void mtk_drm_crtc_atomic_flush(struct drm_crtc *crtc,
}
if (pending_planes)
mtk_crtc->pending_planes = true;
+
+ if (priv->data->shadow_register) {
+ mtk_disp_mutex_acquire(mtk_crtc->mutex);
+ mtk_crtc_ddp_config(crtc);
+ mtk_disp_mutex_release(mtk_crtc->mutex);
+ }
+
if (crtc->state->color_mgmt_changed)
for (i = 0; i < mtk_crtc->ddp_comp_nr; i++)
mtk_ddp_gamma_set(mtk_crtc->ddp_comp[i], crtc->state);
@@ -471,36 +515,10 @@ static int mtk_drm_crtc_init(struct drm_device *drm,
void mtk_crtc_ddp_irq(struct drm_crtc *crtc, struct mtk_ddp_comp *ovl)
{
struct mtk_drm_crtc *mtk_crtc = to_mtk_crtc(crtc);
- struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
- unsigned int i;
+ struct mtk_drm_private *priv = crtc->dev->dev_private;
- /*
- * TODO: instead of updating the registers here, we should prepare
- * working registers in atomic_commit and let the hardware command
- * queue update module registers on vblank.
- */
- if (state->pending_config) {
- mtk_ddp_comp_config(ovl, state->pending_width,
- state->pending_height,
- state->pending_vrefresh, 0);
-
- state->pending_config = false;
- }
-
- if (mtk_crtc->pending_planes) {
- for (i = 0; i < OVL_LAYER_NR; i++) {
- struct drm_plane *plane = &mtk_crtc->planes[i];
- struct mtk_plane_state *plane_state;
-
- plane_state = to_mtk_plane_state(plane->state);
-
- if (plane_state->pending.config) {
- mtk_ddp_comp_layer_config(ovl, i, plane_state);
- plane_state->pending.config = false;
- }
- }
- mtk_crtc->pending_planes = false;
- }
+ if (!priv->data->shadow_register)
+ mtk_crtc_ddp_config(crtc);
mtk_drm_finish_page_flip(mtk_crtc);
}
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index 8030769..b77d456 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -12,6 +12,7 @@
*/
#include <linux/clk.h>
+#include <linux/iopoll.h>
#include <linux/module.h>
#include <linux/of_device.h>
#include <linux/platform_device.h>
@@ -32,10 +33,13 @@
#define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100
#define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n))
+#define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n))
#define DISP_REG_MUTEX_RST(n) (0x28 + 0x20 * (n))
#define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n))
#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n))
+#define INT_MUTEX BIT(1)
+
#define MT8173_MUTEX_MOD_DISP_OVL0 BIT(11)
#define MT8173_MUTEX_MOD_DISP_OVL1 BIT(12)
#define MT8173_MUTEX_MOD_DISP_RDMA0 BIT(13)
@@ -300,6 +304,27 @@ void mtk_disp_mutex_disable(struct mtk_disp_mutex *mutex)
writel(0, ddp->regs + DISP_REG_MUTEX_EN(mutex->id));
}
+void mtk_disp_mutex_acquire(struct mtk_disp_mutex *mutex)
+{
+ struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,
+ mutex[mutex->id]);
+ u32 tmp;
+
+ writel(1, ddp->regs + DISP_REG_MUTEX_EN(mutex->id));
+ writel(1, ddp->regs + DISP_REG_MUTEX(mutex->id));
+ if (readl_poll_timeout_atomic(ddp->regs + DISP_REG_MUTEX(mutex->id),
+ tmp, tmp & INT_MUTEX, 1, 10000))
+ pr_err("could not acquire mutex %d\n", mutex->id);
+}
+
+void mtk_disp_mutex_release(struct mtk_disp_mutex *mutex)
+{
+ struct mtk_ddp *ddp = container_of(mutex, struct mtk_ddp,
+ mutex[mutex->id]);
+
+ writel(0, ddp->regs + DISP_REG_MUTEX(mutex->id));
+}
+
static int mtk_ddp_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
index 92c1175..f9a7991 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
@@ -37,5 +37,7 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
enum mtk_ddp_comp_id id);
void mtk_disp_mutex_unprepare(struct mtk_disp_mutex *mutex);
void mtk_disp_mutex_put(struct mtk_disp_mutex *mutex);
+void mtk_disp_mutex_acquire(struct mtk_disp_mutex *mutex);
+void mtk_disp_mutex_release(struct mtk_disp_mutex *mutex);
#endif /* MTK_DRM_DDP_H */
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index fa0b106..94f8b66 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -33,6 +33,7 @@ struct mtk_mmsys_driver_data {
unsigned int main_len;
const enum mtk_ddp_comp_id *ext_path;
unsigned int ext_len;
+ bool shadow_register;
};
struct mtk_drm_private {
--
1.9.1
^ permalink raw reply related
* [PATCH v9 02/10] drm/mediatek: add *driver_data for different hardware settings
From: YT Shen @ 2016-11-11 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478865346-19043-1-git-send-email-yt.shen@mediatek.com>
There are some hardware settings changed, between MT8173 & MT2701:
DISP_OVL address offset changed, color format definition changed.
DISP_RDMA fifo size changed.
DISP_COLOR offset changed.
MIPI_TX pll setting changed.
And add prefix for mtk_ddp_main & mtk_ddp_ext & mutex_mod.
Signed-off-by: YT Shen <yt.shen@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 27 ++++++++++++++++-----------
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 11 +++++++++--
drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 11 +++++++----
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 +++++++++++++++++++++------
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +++++++++++++
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 25 ++++++++++++++++++-------
drivers/gpu/drm/mediatek/mtk_drm_drv.h | 8 ++++++++
drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 24 +++++++++++++++++++++++-
8 files changed, 115 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
index 019b7ca..1139834 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
@@ -35,13 +35,10 @@
#define DISP_REG_OVL_PITCH(n) (0x0044 + 0x20 * (n))
#define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
#define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
-#define DISP_REG_OVL_ADDR(n) (0x0f40 + 0x20 * (n))
#define OVL_RDMA_MEM_GMC 0x40402020
#define OVL_CON_BYTE_SWAP BIT(24)
-#define OVL_CON_CLRFMT_RGB565 (0 << 12)
-#define OVL_CON_CLRFMT_RGB888 (1 << 12)
#define OVL_CON_CLRFMT_RGBA8888 (2 << 12)
#define OVL_CON_CLRFMT_ARGB8888 (3 << 12)
#define OVL_CON_AEN BIT(8)
@@ -137,18 +134,18 @@ static void mtk_ovl_layer_off(struct mtk_ddp_comp *comp, unsigned int idx)
writel(0x0, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
}
-static unsigned int ovl_fmt_convert(unsigned int fmt)
+static unsigned int ovl_fmt_convert(struct mtk_ddp_comp *comp, unsigned int fmt)
{
switch (fmt) {
default:
case DRM_FORMAT_RGB565:
- return OVL_CON_CLRFMT_RGB565;
+ return comp->data->ovl.fmt_rgb565;
case DRM_FORMAT_BGR565:
- return OVL_CON_CLRFMT_RGB565 | OVL_CON_BYTE_SWAP;
+ return comp->data->ovl.fmt_rgb565 | OVL_CON_BYTE_SWAP;
case DRM_FORMAT_RGB888:
- return OVL_CON_CLRFMT_RGB888;
+ return comp->data->ovl.fmt_rgb888;
case DRM_FORMAT_BGR888:
- return OVL_CON_CLRFMT_RGB888 | OVL_CON_BYTE_SWAP;
+ return comp->data->ovl.fmt_rgb888 | OVL_CON_BYTE_SWAP;
case DRM_FORMAT_RGBX8888:
case DRM_FORMAT_RGBA8888:
return OVL_CON_CLRFMT_ARGB8888;
@@ -178,7 +175,7 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
if (!pending->enable)
mtk_ovl_layer_off(comp, idx);
- con = ovl_fmt_convert(fmt);
+ con = ovl_fmt_convert(comp, fmt);
if (idx != 0)
con |= OVL_CON_AEN | OVL_CON_ALPHA;
@@ -186,7 +183,8 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
writel_relaxed(pitch, comp->regs + DISP_REG_OVL_PITCH(idx));
writel_relaxed(src_size, comp->regs + DISP_REG_OVL_SRC_SIZE(idx));
writel_relaxed(offset, comp->regs + DISP_REG_OVL_OFFSET(idx));
- writel_relaxed(addr, comp->regs + DISP_REG_OVL_ADDR(idx));
+ writel_relaxed(addr, comp->regs + comp->data->ovl.addr_offset
+ + idx * 0x20);
if (pending->enable)
mtk_ovl_layer_on(comp, idx);
@@ -270,6 +268,8 @@ static int mtk_disp_ovl_probe(struct platform_device *pdev)
return ret;
}
+ priv->ddp_comp.data = of_device_get_match_data(dev);
+
platform_set_drvdata(pdev, priv);
ret = component_add(dev, &mtk_disp_ovl_component_ops);
@@ -286,8 +286,13 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev)
return 0;
}
+static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = {
+ .ovl = {0x0f40, 0, 1 << 12}
+};
+
static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
- { .compatible = "mediatek,mt8173-disp-ovl", },
+ { .compatible = "mediatek,mt8173-disp-ovl",
+ .data = &mt8173_ovl_driver_data},
{},
};
MODULE_DEVICE_TABLE(of, mtk_disp_ovl_driver_dt_match);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index 0df05f9..b4225e2 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -123,7 +123,7 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
*/
threshold = width * height * vrefresh * 4 * 7 / 1000000;
reg = RDMA_FIFO_UNDERFLOW_EN |
- RDMA_FIFO_PSEUDO_SIZE(SZ_8K) |
+ RDMA_FIFO_PSEUDO_SIZE(comp->data->rdma_fifo_pseudo_size) |
RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);
writel(reg, comp->regs + DISP_REG_RDMA_FIFO_CON);
}
@@ -208,6 +208,8 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
return ret;
}
+ priv->ddp_comp.data = of_device_get_match_data(dev);
+
platform_set_drvdata(pdev, priv);
ret = component_add(dev, &mtk_disp_rdma_component_ops);
@@ -224,8 +226,13 @@ static int mtk_disp_rdma_remove(struct platform_device *pdev)
return 0;
}
+static const struct mtk_ddp_comp_driver_data mt8173_rdma_driver_data = {
+ .rdma_fifo_pseudo_size = SZ_8K,
+};
+
static const struct of_device_id mtk_disp_rdma_driver_dt_match[] = {
- { .compatible = "mediatek,mt8173-disp-rdma", },
+ { .compatible = "mediatek,mt8173-disp-rdma",
+ .data = &mt8173_rdma_driver_data},
{},
};
MODULE_DEVICE_TABLE(of, mtk_disp_rdma_driver_dt_match);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index 2fc4321..8030769 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -77,9 +77,10 @@ struct mtk_ddp {
struct clk *clk;
void __iomem *regs;
struct mtk_disp_mutex mutex[10];
+ const unsigned int *mutex_mod;
};
-static const unsigned int mutex_mod[DDP_COMPONENT_ID_MAX] = {
+static const unsigned int mt8173_mutex_mod[DDP_COMPONENT_ID_MAX] = {
[DDP_COMPONENT_AAL] = MT8173_MUTEX_MOD_DISP_AAL,
[DDP_COMPONENT_COLOR0] = MT8173_MUTEX_MOD_DISP_COLOR0,
[DDP_COMPONENT_COLOR1] = MT8173_MUTEX_MOD_DISP_COLOR1,
@@ -247,7 +248,7 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
break;
default:
reg = readl_relaxed(ddp->regs + DISP_REG_MUTEX_MOD(mutex->id));
- reg |= mutex_mod[id];
+ reg |= ddp->mutex_mod[id];
writel_relaxed(reg, ddp->regs + DISP_REG_MUTEX_MOD(mutex->id));
return;
}
@@ -273,7 +274,7 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
break;
default:
reg = readl_relaxed(ddp->regs + DISP_REG_MUTEX_MOD(mutex->id));
- reg &= ~mutex_mod[id];
+ reg &= ~(ddp->mutex_mod[id]);
writel_relaxed(reg, ddp->regs + DISP_REG_MUTEX_MOD(mutex->id));
break;
}
@@ -326,6 +327,8 @@ static int mtk_ddp_probe(struct platform_device *pdev)
return PTR_ERR(ddp->regs);
}
+ ddp->mutex_mod = of_device_get_match_data(dev);
+
platform_set_drvdata(pdev, ddp);
return 0;
@@ -337,7 +340,7 @@ static int mtk_ddp_remove(struct platform_device *pdev)
}
static const struct of_device_id ddp_driver_dt_match[] = {
- { .compatible = "mediatek,mt8173-disp-mutex" },
+ { .compatible = "mediatek,mt8173-disp-mutex", .data = mt8173_mutex_mod},
{},
};
MODULE_DEVICE_TABLE(of, ddp_driver_dt_match);
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index df33b3c..661a4a0 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -39,9 +39,8 @@
#define DISP_REG_UFO_START 0x0000
#define DISP_COLOR_CFG_MAIN 0x0400
-#define DISP_COLOR_START 0x0c00
-#define DISP_COLOR_WIDTH 0x0c50
-#define DISP_COLOR_HEIGHT 0x0c54
+#define DISP_COLOR_WIDTH 0x50
+#define DISP_COLOR_HEIGHT 0x54
#define DISP_AAL_EN 0x0000
#define DISP_AAL_SIZE 0x0030
@@ -107,15 +106,15 @@ static void mtk_color_config(struct mtk_ddp_comp *comp, unsigned int w,
unsigned int h, unsigned int vrefresh,
unsigned int bpc)
{
- writel(w, comp->regs + DISP_COLOR_WIDTH);
- writel(h, comp->regs + DISP_COLOR_HEIGHT);
+ writel(w, comp->regs + comp->data->color_offset + DISP_COLOR_WIDTH);
+ writel(h, comp->regs + comp->data->color_offset + DISP_COLOR_HEIGHT);
}
static void mtk_color_start(struct mtk_ddp_comp *comp)
{
writel(COLOR_BYPASS_ALL | COLOR_SEQ_SEL,
comp->regs + DISP_COLOR_CFG_MAIN);
- writel(0x1, comp->regs + DISP_COLOR_START);
+ writel(0x1, comp->regs + comp->data->color_offset);
}
static void mtk_od_config(struct mtk_ddp_comp *comp, unsigned int w,
@@ -264,6 +263,16 @@ struct mtk_ddp_comp_match {
[DDP_COMPONENT_WDMA1] = { MTK_DISP_WDMA, 1, NULL },
};
+static const struct mtk_ddp_comp_driver_data mt8173_color_driver_data = {
+ .color_offset = 0x0c00,
+};
+
+static const struct of_device_id mtk_disp_color_driver_dt_match[] = {
+ { .compatible = "mediatek,mt8173-disp-color",
+ .data = &mt8173_color_driver_data},
+ {},
+};
+
int mtk_ddp_comp_get_id(struct device_node *node,
enum mtk_ddp_comp_type comp_type)
{
@@ -286,6 +295,7 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
enum mtk_ddp_comp_type type;
struct device_node *larb_node;
struct platform_device *larb_pdev;
+ const struct of_device_id *match;
if (comp_id < 0 || comp_id >= DDP_COMPONENT_ID_MAX)
return -EINVAL;
@@ -310,6 +320,11 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
type = mtk_ddp_matches[comp_id].type;
+ if (type == MTK_DISP_COLOR) {
+ match = of_match_node(mtk_disp_color_driver_dt_match, node);
+ comp->data = match->data;
+ }
+
/* Only DMA capable components need the LARB property */
comp->larb_dev = NULL;
if (type != MTK_DISP_OVL &&
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
index 22a33ee..2f6872a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
@@ -78,6 +78,18 @@ struct mtk_ddp_comp_funcs {
struct drm_crtc_state *state);
};
+struct mtk_ddp_comp_driver_data {
+ union {
+ struct ovl {
+ unsigned int addr_offset;
+ unsigned int fmt_rgb565;
+ unsigned int fmt_rgb888;
+ } ovl;
+ unsigned int rdma_fifo_pseudo_size;
+ unsigned int color_offset;
+ };
+};
+
struct mtk_ddp_comp {
struct clk *clk;
void __iomem *regs;
@@ -85,6 +97,7 @@ struct mtk_ddp_comp {
struct device *larb_dev;
enum mtk_ddp_comp_id id;
const struct mtk_ddp_comp_funcs *funcs;
+ const struct mtk_ddp_comp_driver_data *data;
};
static inline void mtk_ddp_comp_config(struct mtk_ddp_comp *comp,
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index cf83f65..5f9b5e8 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -126,7 +126,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
.atomic_commit = mtk_atomic_commit,
};
-static const enum mtk_ddp_comp_id mtk_ddp_main[] = {
+static const enum mtk_ddp_comp_id mt8173_mtk_ddp_main[] = {
DDP_COMPONENT_OVL0,
DDP_COMPONENT_COLOR0,
DDP_COMPONENT_AAL,
@@ -137,7 +137,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
DDP_COMPONENT_PWM0,
};
-static const enum mtk_ddp_comp_id mtk_ddp_ext[] = {
+static const enum mtk_ddp_comp_id mt8173_mtk_ddp_ext[] = {
DDP_COMPONENT_OVL1,
DDP_COMPONENT_COLOR1,
DDP_COMPONENT_GAMMA,
@@ -145,6 +145,13 @@ static int mtk_atomic_commit(struct drm_device *drm,
DDP_COMPONENT_DPI0,
};
+static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
+ .main_path = mt8173_mtk_ddp_main,
+ .main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
+ .ext_path = mt8173_mtk_ddp_ext,
+ .ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
+};
+
static int mtk_drm_kms_init(struct drm_device *drm)
{
struct mtk_drm_private *private = drm->dev_private;
@@ -187,17 +194,19 @@ static int mtk_drm_kms_init(struct drm_device *drm)
* and each statically assigned to a crtc:
* OVL0 -> COLOR0 -> AAL -> OD -> RDMA0 -> UFOE -> DSI0 ...
*/
- ret = mtk_drm_crtc_create(drm, mtk_ddp_main, ARRAY_SIZE(mtk_ddp_main));
+ ret = mtk_drm_crtc_create(drm, private->data->main_path,
+ private->data->main_len);
if (ret < 0)
goto err_component_unbind;
/* ... and OVL1 -> COLOR1 -> GAMMA -> RDMA1 -> DPI0. */
- ret = mtk_drm_crtc_create(drm, mtk_ddp_ext, ARRAY_SIZE(mtk_ddp_ext));
+ ret = mtk_drm_crtc_create(drm, private->data->ext_path,
+ private->data->ext_len);
if (ret < 0)
goto err_component_unbind;
/* Use OVL device for all DMA memory allocations */
- np = private->comp_node[mtk_ddp_main[0]] ?:
- private->comp_node[mtk_ddp_ext[0]];
+ np = private->comp_node[private->data->main_path[0]] ?:
+ private->comp_node[private->data->ext_path[0]];
pdev = of_find_device_by_node(np);
if (!pdev) {
ret = -ENODEV;
@@ -362,6 +371,7 @@ static int mtk_drm_probe(struct platform_device *pdev)
mutex_init(&private->commit.lock);
INIT_WORK(&private->commit.work, mtk_atomic_work);
+ private->data = of_device_get_match_data(dev);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
private->config_regs = devm_ioremap_resource(dev, mem);
@@ -512,7 +522,8 @@ static SIMPLE_DEV_PM_OPS(mtk_drm_pm_ops, mtk_drm_sys_suspend,
mtk_drm_sys_resume);
static const struct of_device_id mtk_drm_of_ids[] = {
- { .compatible = "mediatek,mt8173-mmsys", },
+ { .compatible = "mediatek,mt8173-mmsys",
+ .data = &mt8173_mmsys_driver_data},
{ }
};
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
index aa93894..fa0b106 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
@@ -28,6 +28,13 @@
struct drm_property;
struct regmap;
+struct mtk_mmsys_driver_data {
+ const enum mtk_ddp_comp_id *main_path;
+ unsigned int main_len;
+ const enum mtk_ddp_comp_id *ext_path;
+ unsigned int ext_len;
+};
+
struct mtk_drm_private {
struct drm_device *drm;
struct device *dma_dev;
@@ -40,6 +47,7 @@ struct mtk_drm_private {
void __iomem *config_regs;
struct device_node *comp_node[DDP_COMPONENT_ID_MAX];
struct mtk_ddp_comp *ddp_comp[DDP_COMPONENT_ID_MAX];
+ const struct mtk_mmsys_driver_data *data;
struct {
struct drm_atomic_state *state;
diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
index 1c366f8..935a8ef 100644
--- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
+++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
@@ -16,6 +16,7 @@
#include <linux/delay.h>
#include <linux/io.h>
#include <linux/module.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/phy/phy.h>
@@ -87,6 +88,9 @@
#define MIPITX_DSI_PLL_CON2 0x58
+#define MIPITX_DSI_PLL_TOP 0x64
+#define RG_DSI_MPPLL_PRESERVE (0xff << 8)
+
#define MIPITX_DSI_PLL_PWR 0x68
#define RG_DSI_MPPLL_SDM_PWR_ON BIT(0)
#define RG_DSI_MPPLL_SDM_ISO_EN BIT(1)
@@ -123,10 +127,15 @@
#define SW_LNT2_HSTX_PRE_OE BIT(24)
#define SW_LNT2_HSTX_OE BIT(25)
+struct mtk_mipitx_data {
+ const u32 data;
+};
+
struct mtk_mipi_tx {
struct device *dev;
void __iomem *regs;
unsigned int data_rate;
+ const struct mtk_mipitx_data *driver_data;
struct clk_hw pll_hw;
struct clk *pll;
};
@@ -243,6 +252,10 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON1,
RG_DSI_MPPLL_SDM_SSC_EN);
+ mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_TOP,
+ RG_DSI_MPPLL_PRESERVE,
+ mipi_tx->driver_data->data);
+
return 0;
}
@@ -255,6 +268,9 @@ static void mtk_mipi_tx_pll_unprepare(struct clk_hw *hw)
mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
RG_DSI_MPPLL_PLL_EN);
+ mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_TOP,
+ RG_DSI_MPPLL_PRESERVE, 0);
+
mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_PWR,
RG_DSI_MPPLL_SDM_ISO_EN |
RG_DSI_MPPLL_SDM_PWR_ON,
@@ -391,6 +407,7 @@ static int mtk_mipi_tx_probe(struct platform_device *pdev)
if (!mipi_tx)
return -ENOMEM;
+ mipi_tx->driver_data = of_device_get_match_data(dev);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
mipi_tx->regs = devm_ioremap_resource(dev, mem);
if (IS_ERR(mipi_tx->regs)) {
@@ -448,8 +465,13 @@ static int mtk_mipi_tx_remove(struct platform_device *pdev)
return 0;
}
+static const struct mtk_mipitx_data mt8173_mipitx_data = {
+ .data = (0 << 8)
+};
+
static const struct of_device_id mtk_mipi_tx_match[] = {
- { .compatible = "mediatek,mt8173-mipi-tx", },
+ { .compatible = "mediatek,mt8173-mipi-tx",
+ .data = &mt8173_mipitx_data },
{},
};
--
1.9.1
^ permalink raw reply related
* [PATCH v9 01/10] drm/mediatek: rename macros, add chip prefix
From: YT Shen @ 2016-11-11 11:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478865346-19043-1-git-send-email-yt.shen@mediatek.com>
Add MT8173 prefix for hardware related macros.
Signed-off-by: YT Shen <yt.shen@mediatek.com>
---
drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 60 +++++++++++++++++-----------------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
index 17ba935..2fc4321 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
@@ -36,21 +36,21 @@
#define DISP_REG_MUTEX_MOD(n) (0x2c + 0x20 * (n))
#define DISP_REG_MUTEX_SOF(n) (0x30 + 0x20 * (n))
-#define MUTEX_MOD_DISP_OVL0 BIT(11)
-#define MUTEX_MOD_DISP_OVL1 BIT(12)
-#define MUTEX_MOD_DISP_RDMA0 BIT(13)
-#define MUTEX_MOD_DISP_RDMA1 BIT(14)
-#define MUTEX_MOD_DISP_RDMA2 BIT(15)
-#define MUTEX_MOD_DISP_WDMA0 BIT(16)
-#define MUTEX_MOD_DISP_WDMA1 BIT(17)
-#define MUTEX_MOD_DISP_COLOR0 BIT(18)
-#define MUTEX_MOD_DISP_COLOR1 BIT(19)
-#define MUTEX_MOD_DISP_AAL BIT(20)
-#define MUTEX_MOD_DISP_GAMMA BIT(21)
-#define MUTEX_MOD_DISP_UFOE BIT(22)
-#define MUTEX_MOD_DISP_PWM0 BIT(23)
-#define MUTEX_MOD_DISP_PWM1 BIT(24)
-#define MUTEX_MOD_DISP_OD BIT(25)
+#define MT8173_MUTEX_MOD_DISP_OVL0 BIT(11)
+#define MT8173_MUTEX_MOD_DISP_OVL1 BIT(12)
+#define MT8173_MUTEX_MOD_DISP_RDMA0 BIT(13)
+#define MT8173_MUTEX_MOD_DISP_RDMA1 BIT(14)
+#define MT8173_MUTEX_MOD_DISP_RDMA2 BIT(15)
+#define MT8173_MUTEX_MOD_DISP_WDMA0 BIT(16)
+#define MT8173_MUTEX_MOD_DISP_WDMA1 BIT(17)
+#define MT8173_MUTEX_MOD_DISP_COLOR0 BIT(18)
+#define MT8173_MUTEX_MOD_DISP_COLOR1 BIT(19)
+#define MT8173_MUTEX_MOD_DISP_AAL BIT(20)
+#define MT8173_MUTEX_MOD_DISP_GAMMA BIT(21)
+#define MT8173_MUTEX_MOD_DISP_UFOE BIT(22)
+#define MT8173_MUTEX_MOD_DISP_PWM0 BIT(23)
+#define MT8173_MUTEX_MOD_DISP_PWM1 BIT(24)
+#define MT8173_MUTEX_MOD_DISP_OD BIT(25)
#define MUTEX_SOF_SINGLE_MODE 0
#define MUTEX_SOF_DSI0 1
@@ -80,21 +80,21 @@ struct mtk_ddp {
};
static const unsigned int mutex_mod[DDP_COMPONENT_ID_MAX] = {
- [DDP_COMPONENT_AAL] = MUTEX_MOD_DISP_AAL,
- [DDP_COMPONENT_COLOR0] = MUTEX_MOD_DISP_COLOR0,
- [DDP_COMPONENT_COLOR1] = MUTEX_MOD_DISP_COLOR1,
- [DDP_COMPONENT_GAMMA] = MUTEX_MOD_DISP_GAMMA,
- [DDP_COMPONENT_OD] = MUTEX_MOD_DISP_OD,
- [DDP_COMPONENT_OVL0] = MUTEX_MOD_DISP_OVL0,
- [DDP_COMPONENT_OVL1] = MUTEX_MOD_DISP_OVL1,
- [DDP_COMPONENT_PWM0] = MUTEX_MOD_DISP_PWM0,
- [DDP_COMPONENT_PWM1] = MUTEX_MOD_DISP_PWM1,
- [DDP_COMPONENT_RDMA0] = MUTEX_MOD_DISP_RDMA0,
- [DDP_COMPONENT_RDMA1] = MUTEX_MOD_DISP_RDMA1,
- [DDP_COMPONENT_RDMA2] = MUTEX_MOD_DISP_RDMA2,
- [DDP_COMPONENT_UFOE] = MUTEX_MOD_DISP_UFOE,
- [DDP_COMPONENT_WDMA0] = MUTEX_MOD_DISP_WDMA0,
- [DDP_COMPONENT_WDMA1] = MUTEX_MOD_DISP_WDMA1,
+ [DDP_COMPONENT_AAL] = MT8173_MUTEX_MOD_DISP_AAL,
+ [DDP_COMPONENT_COLOR0] = MT8173_MUTEX_MOD_DISP_COLOR0,
+ [DDP_COMPONENT_COLOR1] = MT8173_MUTEX_MOD_DISP_COLOR1,
+ [DDP_COMPONENT_GAMMA] = MT8173_MUTEX_MOD_DISP_GAMMA,
+ [DDP_COMPONENT_OD] = MT8173_MUTEX_MOD_DISP_OD,
+ [DDP_COMPONENT_OVL0] = MT8173_MUTEX_MOD_DISP_OVL0,
+ [DDP_COMPONENT_OVL1] = MT8173_MUTEX_MOD_DISP_OVL1,
+ [DDP_COMPONENT_PWM0] = MT8173_MUTEX_MOD_DISP_PWM0,
+ [DDP_COMPONENT_PWM1] = MT8173_MUTEX_MOD_DISP_PWM1,
+ [DDP_COMPONENT_RDMA0] = MT8173_MUTEX_MOD_DISP_RDMA0,
+ [DDP_COMPONENT_RDMA1] = MT8173_MUTEX_MOD_DISP_RDMA1,
+ [DDP_COMPONENT_RDMA2] = MT8173_MUTEX_MOD_DISP_RDMA2,
+ [DDP_COMPONENT_UFOE] = MT8173_MUTEX_MOD_DISP_UFOE,
+ [DDP_COMPONENT_WDMA0] = MT8173_MUTEX_MOD_DISP_WDMA0,
+ [DDP_COMPONENT_WDMA1] = MT8173_MUTEX_MOD_DISP_WDMA1,
};
static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
--
1.9.1
^ permalink raw reply related
* [PATCH v9 00/10] MT2701 DRM support
From: YT Shen @ 2016-11-11 11:55 UTC (permalink / raw)
To: linux-arm-kernel
This is MT2701 DRM support PATCH v9, based on 4.9-rc1.
We add DSI interrupt control, transfer function for MIPI DSI panel support.
Most codes are the same, except some register changed.
For example:
- DISP_OVL address offset changed, color format definition changed.
- DISP_RDMA fifo size changed.
- DISP_COLOR offset changed.
- MIPI_TX setting changed.
We add a new component DDP_COMPONENT_BLS, and the connections are updated.
OVL -> RDMA -> COLOR -> BLS -> DSI
RDMA -> DPI
And we have shadow register support in MT2701.
We remove dts patch from the patch series, which depends on MT2701 CCF and scpsys.
Changes since v8:
- enable 3 DSI interrupts only
- move mtk_dsi_wait_for_irq_done() to the patch of irq control
- use the name BLS in DRM driver part
- move BLS declaration to a separate patch
- update mtk_dsi_switch_to_cmd_mode()
- update mtk_output_dsi_enable() and mtk_output_dsi_disable()
Changes since v7:
- Remove redundant codes
- Move the definition of DDP_COMPONENT_BLS to patch of "drm/mediatek: update display module connections"
- Move _dsi_irq_wait_queue into platform driver data
- Move mtk_dsi_irq_data_clear() to patch of "drm/mediatek: add dsi interrupt control"
- Add more descriptions in the commit messages
Changes since v6:
- Change data type of irq_data to u32
- Rewrite mtk_dsi_host_transfer() for simplify
- Move some MIPI_TX config to patch of "drm/mediatek: add *driver_data for different hardware settings".
- Remove device tree from this patch series
Changes since v5:
- Remove DPI device tree and compatible string
- Use one wait queue to handle interrupt status
- Update the interrupt check flow and DSI_INT_ALL_BITS
- Use same function for host read/write command
- various fixes
Changes since v4:
- Add messages when timeout in mtk_disp_mutex_acquire()
- Add descriptions for DISP_REG_MUTEX registers
- Move connection settings for display modules to a separate patch
- Remove 'mt2701-disp-wdma' because it is unused
- Move cleaning up and renaming to a separate patch
- Use wait_event_interruptible_timeout() to replace polling
- Remove irq_num from mtk_dsi structure
- Remove redundant and debug codes
Changes since v3:
- Add DSI support for MIPI DSI panels
- Update BLS binding to PWM nodes
- Remove ufoe device nodes
- Remove redundant parentheses
- Remove global variable initialization
Changes since v2:
- Rename mtk_ddp_mux_sel to mtk_ddp_sout_sel
- Update mt2701_mtk_ddp_ext components
- Changed to prefix naming
- Reorder the patch series
- Use of_device_get_match_data() to get driver private data
- Use iopoll macros to implement mtk_disp_mutex_acquire()
- Removed empty device tree nodes
Changes since v1:
- Removed BLS bindings and codes, which belong to pwm driver
- Moved mtk_disp_mutex_acquire() just before mtk_crtc_ddp_config()
- Split patch into smaller parts
- Added const keyword to constant structure
- Removed codes for special memory align
Thanks,
yt.shen
YT Shen (8):
drm/mediatek: rename macros, add chip prefix
drm/mediatek: add *driver_data for different hardware settings
drm/mediatek: add shadow register support
drm/mediatek: add BLS component
drm/mediatek: update display module connections
drm/mediatek: cleaning up and refine
drm/mediatek: update DSI sub driver flow for sending commands to panel
drm/mediatek: add support for Mediatek SoC MT2701
shaoming chen (2):
drm/mediatek: add dsi interrupt control
drm/mediatek: add dsi transfer function
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 33 ++-
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 17 +-
drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 76 +++--
drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 138 ++++++---
drivers/gpu/drm/mediatek/mtk_drm_ddp.h | 2 +
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 38 ++-
drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 15 +
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 54 +++-
drivers/gpu/drm/mediatek/mtk_drm_drv.h | 9 +
drivers/gpu/drm/mediatek/mtk_dsi.c | 429 ++++++++++++++++++++++++----
drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 70 +++--
11 files changed, 715 insertions(+), 166 deletions(-)
--
1.9.1
^ 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-11 11:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <5825A927.2000202@gmail.com>
On Fri, Nov 11, 2016 at 04:49:03PM +0530, Anurup M wrote:
> On Thursday 10 November 2016 10:53 PM, Mark Rutland wrote:
> >On Thu, Nov 03, 2016 at 01:41:58AM -0400, Anurup M wrote:
> >>diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon.txt
> >>+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 L3 cache PMU bindings are defined @bindings/arm/hisilicon/pmu.txt.
> Is it OK that I document here(hisilicon/djtag.txt), a reference to
> the PMU bindings doc ?
At this point in the series, that file does not exist yet, and this is
an undocumented beinding.
Please introduce this sub-node long with the PMU bindings, later in the
series.
Thanks,
Mark.
^ permalink raw reply
* [PATCH] PM / Domains: Fix compatible for domain idle state
From: Ulf Hansson @ 2016-11-11 11:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110195832.4nz7lxlmshaemcbb@rob-hp-laptop>
On 10 November 2016 at 20:58, Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 07, 2016 at 12:14:28PM +0100, Ulf Hansson wrote:
> > On 3 November 2016 at 22:54, Lina Iyer <lina.iyer@linaro.org> wrote:
> > > Re-using idle state definition provided by arm,idle-state for domain
> > > idle states creates a lot of confusion and limits further evolution of
> > > the domain idle definition. To keep things clear and simple, define a
> > > idle states for domain using a new compatible "domain-idle-state".
> > >
> > > Fix existing PM domains code to look for the newly defined compatible.
> > >
> > > Cc: <devicetree@vger.kernel.org>
> > > Cc: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> > > ---
> > > .../bindings/power/domain-idle-state.txt | 33 ++++++++++++++++++++++
> > > .../devicetree/bindings/power/power_domain.txt | 8 +++---
> > > drivers/base/power/domain.c | 2 +-
> > > 3 files changed, 38 insertions(+), 5 deletions(-)
> > > create mode 100644 Documentation/devicetree/bindings/power/domain-idle-state.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/power/domain-idle-state.txt b/Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > new file mode 100644
> > > index 0000000..eefc7ed
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/power/domain-idle-state.txt
> > > @@ -0,0 +1,33 @@
> > > +PM Domain Idle State Node:
> > > +
> > > +A domain idle state node represents the state parameters that will be used to
> > > +select the state when there are no active components in the domain.
> > > +
> > > +The state node has the following parameters -
> > > +
> > > +- compatible:
> > > + Usage: Required
> > > + Value type: <string>
> > > + Definition: Must be "domain-idle-state".
> > > +
> > > +- entry-latency-us
> > > + Usage: Required
> > > + Value type: <prop-encoded-array>
> > > + Definition: u32 value representing worst case latency in
> > > + microseconds required to enter the idle state.
> > > + The exit-latency-us duration may be guaranteed
> > > + only after entry-latency-us has passed.
> >
> > As we anyway are going to change this, why not use an u64 and have the
> > value in ns instead of us?
>
> I can't imagine that you would need more resolution or range. For times
> less than 1us, s/w and register access times are going to dominate the
> time.
Yep.
>
>
> Unless there is a real need, I'd keep alignment with the existing
> binding.
Agree!
Kind regards
Uffe
^ permalink raw reply
* [PATCH v7] soc: qcom: add l2 cache perf events driver
From: Mark Rutland @ 2016-11-11 11:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <50feb4f2-f042-5f75-732e-5a99653b51f2@codeaurora.org>
On Thu, Nov 10, 2016 at 06:25:47PM -0500, Leeder, Neil wrote:
> On 11/9/2016 12:54 PM, Mark Rutland wrote:
> >>+
> >>+/*
> >>+ * The cache is made up of one or more clusters, each cluster has its own PMU.
> >>+ * Each cluster is associated with one or more CPUs.
> >>+ * This structure represents one of the hardware PMUs.
> >>+ *
> >>+ * Events can be envisioned as a 2-dimensional array. Each column represents
> >>+ * a group of events. There are 8 groups. Only one entry from each
> >>+ * group can be in use at a time. When an event is assigned a counter
> >>+ * by *_event_add(), the counter index is assigned to group_to_counter[group].
> >
> >If I've followed correctly, this means each group has a dedicated
> >counter?
> >
> >I take it groups are not symmetric (i.e. each column has different
> >events)? Or does each column contain the same events?
> >
> >Is there any overlap?
>
> Each group will have at most one counter, but it's not dedicated.
> There's no requirement that an implementation have as many counters
> as there are groups, so an event can be assigned to any available
> counter.
>
> Every entry in the 2-dimensional array is unique, so no duplicates.
> Once you have used an event, you cannot use any other event from its
> column.
Ok; thanks for clarifying that!
> >>+static int l2_cache__event_init(struct perf_event *event)
> >>+ /* Don't allow groups with mixed PMUs, except for s/w events */
> >>+ if (event->group_leader->pmu != event->pmu &&
> >>+ !is_software_event(event->group_leader)) {
> >>+ dev_warn(&l2cache_pmu->pdev->dev,
> >>+ "Can't create mixed PMU group\n");
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ list_for_each_entry(sibling, &event->group_leader->sibling_list,
> >>+ group_entry)
> >>+ if (sibling->pmu != event->pmu &&
> >>+ !is_software_event(sibling)) {
> >>+ dev_warn(&l2cache_pmu->pdev->dev,
> >>+ "Can't create mixed PMU group\n");
> >>+ return -EINVAL;
> >>+ }
> >>+
> >>+ /* Ensure all events in a group are on the same cpu */
> >>+ cluster = get_hml2_pmu(event->cpu);
> >>+ if ((event->group_leader != event) &&
> >>+ (cluster->on_cpu != event->group_leader->cpu)) {
> >>+ dev_warn(&l2cache_pmu->pdev->dev,
> >>+ "Can't create group on CPUs %d and %d",
> >>+ event->cpu, event->group_leader->cpu);
> >>+ return -EINVAL;
> >>+ }
> >
> >It's probably worth also checking that the events are co-schedulable
> >(e.g. they don't conflict column-wise).
>
> That's what filter_match() is doing - stopping column-conflicting
> events from even getting to init(). In init() we don't have a record
> of which other events are being co-scheduled. We could keep a list
> of groups used by other events to compare against, but because
> there's no matching term() function there's no obvious way of
> removing them from the list.
I mean within the group, in addition to the filter_match() logic.
When you event_init() an event, you can determine whether there is any
column conflict within the group the new events is being placed in, and
whether you have sufficient counters to ever be able to schedule that
group. If not, the group should be rejected.
Other PMUs have similar checks; see l2x0_pmu_group_is_valid() in
arch/arm/mm/cache-l2x0-pmu.c, and valiate_group() in
drivers/perf/arm_pmu.c.
I don't believe that filer_match() can catch that, as it's called on
each event in a group individually prior to add() time, and thus there's
no visibility of the group as a whole.
Regardless, we can and should catch that case far earlier.
[...]
> >>+ if (acpi_bus_get_device(ACPI_HANDLE(dev), &device))
> >>+ return -ENODEV;
> >>+
> >>+ if (kstrtol(device->pnp.unique_id, 10, &fw_cluster_id) < 0) {
> >>+ dev_err(&pdev->dev, "unable to read ACPI uid\n");
> >>+ return -ENODEV;
> >>+ }
> >
> >>+ cluster->l2cache_pmu = l2cache_pmu;
> >>+ for_each_present_cpu(cpu) {
> >>+ if (topology_physical_package_id(cpu) == fw_cluster_id) {
> >>+ cpumask_set_cpu(cpu, &cluster->cluster_cpus);
> >>+ per_cpu(pmu_cluster, cpu) = cluster;
> >>+ }
> >>+ }
> >
> >I'm still uneasy about this.
> >
> >The topology_* API doesn't have a well-defined mapping to the MPIDR.Aff*
> >levels, which itself also don't have a well-defined mapping to your
> >hardware's clusters (and therefore fw_cluster_id).
> >
> >Thus, I'm rather worried that this is going to get broken easily, either
> >by changes in the kernel, or in future HW revisions where the mapping of
> >clusters to MPIDR.Aff* fields changes.
>
> I'm not sure how else to get a mapping of CPU to cluster which
> doesn't eventually end with MPIDR.
This is unfortunate. :(
It would have been much nicer if the FW also provided the MPIDR.Aff<n>
level to match up to, as that would be unambiguous.
> This is the definition of topology_physical_package_id() from
> asm/topology.h:
>
> #define topology_physical_package_id(cpu)
> (cpu_topology[cpu].cluster_id)
>
> It seems to be a pretty solid connection between cpu and cluster.
As I mentioned above, there's no well-defined mapping from MPIDR.Aff* to
the topology API levels. The "cluster_id" here is a guess, and one that
might change in future based on other heuristics.
> I don't think this is an abuse of this function. Unless there is some
> other way of getting this mapping I'd suggest using this, and if some
> future chip should change MPIDR usage it can be addressed it then.
I don't think it's an abuse, as such, but I don't think that it is
reliable.
That said, I don't see that we can do any better, as you say.
It's probably worth adding a comment block regarding our expectations,
i.e. that cluster_id means Aff1 for CPUs without multi-threading, Aff2
otherwise, and that we hope future systems don't choose another
MPIDR.Aff* mapping scheme.
Thanks,
Mark.
^ permalink raw reply
* [RFC v2 8/8] iommu/arm-smmu: implement add_reserved_regions callback
From: Joerg Roedel @ 2016-11-11 11:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <f4537e43-0237-e478-eacd-b107458628b8@redhat.com>
On Thu, Nov 10, 2016 at 07:00:52PM +0100, Auger Eric wrote:
> GICv2m and GICV3 ITS use dma-mapping iommu_dma_map_msi_msg to allocate
> an MSI IOVA on-demand.
Yes, and it the right thing to do there because as a DMA-API
implementation the dma-iommu code cares about the address space
allocation.
As I understand it this is different in your case, as someone else is
defining the address space layout. So why do you need to allocate it
yourself?
Joerg
^ permalink raw reply
* ACPI namespace details for ARM64
From: Lorenzo Pieralisi @ 2016-11-11 11:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109220506.GN14322@bhelgaas-glaptop.roam.corp.google.com>
On Wed, Nov 09, 2016 at 04:05:06PM -0600, Bjorn Helgaas wrote:
[...]
> ACPI defined a Producer/Consumer bit that was intended to distinguish
> the bridge apertures from the bridge registers [4, 5]. However, BIOSes
> didn't use that bit correctly, and the result is that OSes have to
> assume that everything in a PCI host bridge _CRS is a window. That
> leaves no way to describe the bridge registers in the PNP0A03/PNP0A08
> device itself.
ACPI 6.1 states that in the revision changes 4.0a Apr.2010 (xiii)
"Consumer/Producer bit is ignored (Restored 2.0C change that had
been lost)" and still that bit is marked as valid. If it is not
reliable it should be set as "ignored" in the specs (as it was
on ACPI 2.0C, BTW), as it is it is just a source of confusion.
Thanks again !
Lorenzo
^ permalink raw reply
* Summary of LPC guest MSI discussion in Santa Fe
From: Joerg Roedel @ 2016-11-11 11:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110104601.0939ba9a@t450s.home>
On Thu, Nov 10, 2016 at 10:46:01AM -0700, Alex Williamson wrote:
> 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.
The drivers actually allow mappings to the MSI region via the IOMMU-API,
and I think it should stay this way also for other reserved ranges.
Address space management is done by the IOMMU-API user already (and has
to be done there nowadays), be it a DMA-API implementation which just
reserves these regions in its address space allocator or be it VFIO with
QEMU, which don't map RAM there anyway. So there is no point of checking
this again in the IOMMU drivers and we can keep that out of the
mapping/unmapping fast-path.
> 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... ???
Right, the hardware resources can be examined. But maybe this can be
extended to also cover RMRR ranges? Then we would be able to assign
devices with RMRR mappings to guests.
Joerg
^ permalink raw reply
* [RESEND PATCH v1 02/11] dt-bindings: hisi: Add Hisilicon HiP05/06/07 Sysctrl and Djtag dts bindings
From: Anurup M @ 2016-11-11 11:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110172320.GA10137@leverpostej>
On Thursday 10 November 2016 10:53 PM, Mark Rutland wrote:
> 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.
Ok. Shall move the djtag bindings to hisilicon/djtag.txt.
>> +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.
Ok. I shall remove "djtag driver" and shall modify as
+The Hisilicon Djtag in CPU die is an independent component which connects with
+some other components in the SoC by Debug Bus. The djtag will use the system controller
+registers to access the connecting components in CPU die (like L3 cache, l3 cache PMU
+etc.) during real time debugging. These components appear as child nodes of djtag.
>> +
>> +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.
My Apologies. The syscon is not used now. Mistake in file version used
for patch creation.
The compatible filed will be "hisilicon,hisi-cpu-djtag-v1"
>> +- 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 L3 cache PMU bindings are defined @bindings/arm/hisilicon/pmu.txt.
Is it OK that I document here(hisilicon/djtag.txt), a reference to the
PMU bindings doc ?
>> + };
>> +
>> +-----------------------------------------------------------------------
>> +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.
The ALGSUB is the name of the sysctrl of IO die. I shall remove ALGSUB
to avoid confusion.
I would also add the below details in the introduction.
The Hisilicon djtag will use the system controller registers to control
access to connecting modules
of CPU and IO die in the chip. There are separate system controller
registers for CPU and IO die in the chip.
And the section will be modified as
+Hisilicon HiP05 djtag for IO sysctrl
+Required properties:
+ - compatible : "hisilicon,hisi-io-djtag-v1"
+ - reg : Register address and size
+
+Hisilicon HiP06/07 djtag for IO sysctrl
+Required properties:
+ - compatible : "hisilicon,hip06-io-djtag-v2"
+ - reg : Register address and size
+
+Example:
+ /* for Hisilicon HiP05 djtag for IO sysctrl */
+ djtag0: djtag at d0000000 {
+ compatible = "hisilicon,hisi-io-djtag-v1";
+ reg = <0x0 0xd0000000 0x0 0x10000>;
+ };
Thanks,
Anurup
> 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
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Mark Rutland @ 2016-11-11 11:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161111103921.GE16907@hardcore>
On Fri, Nov 11, 2016 at 11:39:21AM +0100, Jan Glauber wrote:
> Hi Mark,
>
> thanks for reviewing. One question below,
> On Thu, Nov 10, 2016 at 04:54:06PM +0000, Mark Rutland wrote:
> > On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > > +#include <linux/cpufeature.h>
> > > +#include <linux/numa.h>
> > > +#include <linux/slab.h>
> >
> > I believe the following includes are necessary for APIs and/or data
> > explicitly referenced by the driver code:
[...]
> Should I also add includes that are already in the included by uncore_cavium.h?
Please do.
> I usually avoid includes that come through the "local" header file.
Generally, when you explcitly use some macro/function/data in a file,
that file should have the relevant include.
If something's only used in the header (e.g. hidden in a macro or inline
function), then we only need that include in the header.
For example: uncore_cavium.h uses container_of(), and should include
<linux/kernel.h>. Also, uncore_cavium.c also uses container_of()
directly for something unrelated, and should also include
<linux/kernel.h>.
Thanks,
Mark.
^ permalink raw reply
* [PATCH v6 2/9] drm/hisilicon/hibmc: Add video memory management
From: Rongrong Zou @ 2016-11-11 11:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOw6vbKK+kmjZv+su4tg43Pcos-B3MjZcAv461au86SeqCQP=A@mail.gmail.com>
? 2016/11/11 1:35, Sean Paul ??:
> On Fri, Oct 28, 2016 at 3:27 AM, Rongrong Zou <zourongrong@gmail.com> wrote:
>> Hibmc have 32m video memory which can be accessed through PCIe by host,
>> we use ttm to manage these memory.
>>
>> Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
>> ---
>> drivers/gpu/drm/hisilicon/hibmc/Kconfig | 1 +
>> drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +-
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 +
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 46 +++
>> drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 490 ++++++++++++++++++++++++
>> 5 files changed, 550 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> index a9af90d..bcb8c18 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig
>> @@ -1,6 +1,7 @@
>> config DRM_HISI_HIBMC
>> tristate "DRM Support for Hisilicon Hibmc"
>> depends on DRM && PCI
>> + select DRM_TTM
>>
>> help
>> Choose this option if you have a Hisilicon Hibmc soc chipset.
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> index 97cf4a0..d5c40b8 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile
>> @@ -1,5 +1,5 @@
>> ccflags-y := -Iinclude/drm
>> -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o
>> +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o
>>
>> obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o
>> #obj-y += hibmc-drm.o
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> index 4669d42..81f4301 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
>> @@ -31,6 +31,7 @@
>> #ifdef CONFIG_COMPAT
>> .compat_ioctl = drm_compat_ioctl,
>> #endif
>> + .mmap = hibmc_mmap,
>> .poll = drm_poll,
>> .read = drm_read,
>> .llseek = no_llseek,
>> @@ -46,6 +47,8 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>> }
>>
>> static struct drm_driver hibmc_driver = {
>> + .driver_features = DRIVER_GEM,
>> +
>
> nit: extra space
>
>> .fops = &hibmc_fops,
>> .name = "hibmc",
>> .date = "20160828",
>> @@ -55,6 +58,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe)
>> .get_vblank_counter = drm_vblank_no_hw_counter,
>> .enable_vblank = hibmc_enable_vblank,
>> .disable_vblank = hibmc_disable_vblank,
>> + .gem_free_object_unlocked = hibmc_gem_free_object,
>> + .dumb_create = hibmc_dumb_create,
>> + .dumb_map_offset = hibmc_dumb_mmap_offset,
>> + .dumb_destroy = drm_gem_dumb_destroy,
>> };
>>
>> static int hibmc_pm_suspend(struct device *dev)
>> @@ -163,6 +170,7 @@ static int hibmc_unload(struct drm_device *dev)
>> {
>> struct hibmc_drm_device *hidev = dev->dev_private;
>>
>> + hibmc_mm_fini(hidev);
>> hibmc_hw_fini(hidev);
>> dev->dev_private = NULL;
>> return 0;
>> @@ -183,6 +191,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags)
>> if (ret)
>> goto err;
>>
>> + ret = hibmc_mm_init(hidev);
>> + if (ret)
>> + goto err;
>> +
>> return 0;
>>
>> err:
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index 0037341..db8d80e 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -20,6 +20,8 @@
>> #define HIBMC_DRM_DRV_H
>>
>> #include <drm/drmP.h>
>> +#include <drm/ttm/ttm_bo_driver.h>
>> +#include <drm/drm_gem.h>
>
> nit: alphabetize
will fix it, thanks.
>
>>
>> struct hibmc_drm_device {
>> /* hw */
>> @@ -30,6 +32,50 @@ struct hibmc_drm_device {
>>
>> /* drm */
>> struct drm_device *dev;
>> +
>> + /* ttm */
>> + struct {
>> + struct drm_global_reference mem_global_ref;
>> + struct ttm_bo_global_ref bo_global_ref;
>> + struct ttm_bo_device bdev;
>> + bool initialized;
>> + } ttm;
>
> I don't think you gain anything other than keystrokes from the substruct
I'm sorry i didn't catch you, i looked at the all drivers used ttm such
as ast/bochs/cirrus/mgag200/qxl/virtio_gpu, they all embedded the ttm substruct
into the driver-private struct.
so do you mean
struct hibmc_drm_device {
/* hw */
void __iomem *mmio;
void __iomem *fb_map;
unsigned long fb_base;
unsigned long fb_size;
/* drm */
struct drm_device *dev;
struct drm_plane plane;
struct drm_crtc crtc;
struct drm_encoder encoder;
struct drm_connector connector;
bool mode_config_initialized;
/* ttm */
struct drm_global_reference mem_global_ref;
struct ttm_bo_global_ref bo_global_ref;
struct ttm_bo_device bdev;
bool initialized;
...
};
?
>
>> +
>> + bool mm_inited;
>> };
>>
>> +struct hibmc_bo {
>> + struct ttm_buffer_object bo;
>> + struct ttm_placement placement;
>> + struct ttm_bo_kmap_obj kmap;
>> + struct drm_gem_object gem;
>> + struct ttm_place placements[3];
>> + int pin_count;
>> +};
>> +
>> +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo)
>> +{
>> + return container_of(bo, struct hibmc_bo, bo);
>> +}
>> +
>> +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem)
>> +{
>> + return container_of(gem, struct hibmc_bo, gem);
>> +}
>> +
>> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
>
> Hide this in ttm.c
ok, will do that.
thanks for pointing it out.
>
>> +
>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> + struct drm_gem_object **obj);
>> +
>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc);
>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc);
>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr);
>> +void hibmc_gem_free_object(struct drm_gem_object *obj);
>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> + struct drm_mode_create_dumb *args);
>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>> + u32 handle, u64 *offset);
>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma);
>> +
>> #endif
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> new file mode 100644
>> index 0000000..0802ebd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c
>> @@ -0,0 +1,490 @@
>> +/* Hisilicon Hibmc SoC drm driver
>> + *
>> + * Based on the bochs drm driver.
>> + *
>> + * Copyright (c) 2016 Huawei Limited.
>> + *
>> + * Author:
>> + * Rongrong Zou <zourongrong@huawei.com>
>> + * Rongrong Zou <zourongrong@gmail.com>
>> + * Jianhua Li <lijianhua@huawei.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include "hibmc_drm_drv.h"
>> +#include <ttm/ttm_page_alloc.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_atomic_helper.h>
>> +
>> +static inline struct hibmc_drm_device *
>> +hibmc_bdev(struct ttm_bo_device *bd)
>> +{
>> + return container_of(bd, struct hibmc_drm_device, ttm.bdev);
>> +}
>> +
>> +static int
>> +hibmc_ttm_mem_global_init(struct drm_global_reference *ref)
>> +{
>> + return ttm_mem_global_init(ref->object);
>> +}
>> +
>> +static void
>> +hibmc_ttm_mem_global_release(struct drm_global_reference *ref)
>> +{
>> + ttm_mem_global_release(ref->object);
>> +}
>> +
>> +static int hibmc_ttm_global_init(struct hibmc_drm_device *hibmc)
>> +{
>> + struct drm_global_reference *global_ref;
>> + int r;
>
> nit: try not to use one character variable names unless it's for the
> purpose of a loop (ie: i,j). You also use ret elsewhere in the driver,
> so it'd be nice to remain consistent
the whole file is delivered from bochs ttm, i didn't modify anything except
some checkpatch warnings and the 'hibmc_' prefix. Unfortunately, some
problems were delivered too.
>
>> +
>> + global_ref = &hibmc->ttm.mem_global_ref;
>
> I think using the global_ref local obfuscates what you're doing here.
> It saves you 6 characters while typing, but adds a layer of
> indirection for all future readers.
>
>> + global_ref->global_type = DRM_GLOBAL_TTM_MEM;
>> + global_ref->size = sizeof(struct ttm_mem_global);
>> + global_ref->init = &hibmc_ttm_mem_global_init;
>> + global_ref->release = &hibmc_ttm_mem_global_release;
>> + r = drm_global_item_ref(global_ref);
>> + if (r != 0) {
>
> nit: if (r)
will fix it,
thanks.
BTW, i wonder why checkpatch.pl didn't report it.
>
>> + DRM_ERROR("Failed setting up TTM memory accounting subsystem.\n"
>> + );
>
> Breaking up the line for one character is probably not worthwhile, and
> you should really print the error. How about:
>
> DRM_ERROR("Could not get ref on ttm global ret=%d.\n", ret);
i like your solution, thanks.
>
>
>> + return r;
>> + }
>> +
>> + hibmc->ttm.bo_global_ref.mem_glob =
>> + hibmc->ttm.mem_global_ref.object;
>> + global_ref = &hibmc->ttm.bo_global_ref.ref;
>> + global_ref->global_type = DRM_GLOBAL_TTM_BO;
>> + global_ref->size = sizeof(struct ttm_bo_global);
>> + global_ref->init = &ttm_bo_global_init;
>> + global_ref->release = &ttm_bo_global_release;
>> + r = drm_global_item_ref(global_ref);
>> + if (r != 0) {
>> + DRM_ERROR("Failed setting up TTM BO subsystem.\n");
>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref);
>> + return r;
>> + }
>> + return 0;
>> +}
>> +
>> +static void
>> +hibmc_ttm_global_release(struct hibmc_drm_device *hibmc)
>> +{
>> + if (!hibmc->ttm.mem_global_ref.release)
>
> Are you actually hitting this condition? This seems like it's papering
> over something else.
it was also delivered from others, i looked at the xxx_ttm_global_init
function, 'mem_global_ref.release' is assigned unconditionally, so i
think this condition never be hit, it may be hit when release twice,
but this won't take place in my driver.
>
>> + return;
>> +
>> + drm_global_item_unref(&hibmc->ttm.bo_global_ref.ref);
>> + drm_global_item_unref(&hibmc->ttm.mem_global_ref);
>> + hibmc->ttm.mem_global_ref.release = NULL;
>> +}
>> +
>> +static void hibmc_bo_ttm_destroy(struct ttm_buffer_object *tbo)
>> +{
>> + struct hibmc_bo *bo;
>> +
>> + bo = container_of(tbo, struct hibmc_bo, bo);
>
> nit: No need to split this into a separate line.
agreed, thanks.
>
>> +
>> + drm_gem_object_release(&bo->gem);
>> + kfree(bo);
>> +}
>> +
>> +static bool hibmc_ttm_bo_is_hibmc_bo(struct ttm_buffer_object *bo)
>> +{
>> + if (bo->destroy == &hibmc_bo_ttm_destroy)
>> + return true;
>> + return false;
>
> return bo->destroy == &hibmc_bo_ttm_destroy;
looks better to me.
>
>> +}
>> +
>> +static int
>> +hibmc_bo_init_mem_type(struct ttm_bo_device *bdev, u32 type,
>> + struct ttm_mem_type_manager *man)
>> +{
>> + switch (type) {
>> + case TTM_PL_SYSTEM:
>> + man->flags = TTM_MEMTYPE_FLAG_MAPPABLE;
>> + man->available_caching = TTM_PL_MASK_CACHING;
>> + man->default_caching = TTM_PL_FLAG_CACHED;
>> + break;
>> + case TTM_PL_VRAM:
>> + man->func = &ttm_bo_manager_func;
>> + man->flags = TTM_MEMTYPE_FLAG_FIXED |
>> + TTM_MEMTYPE_FLAG_MAPPABLE;
>> + man->available_caching = TTM_PL_FLAG_UNCACHED |
>> + TTM_PL_FLAG_WC;
>> + man->default_caching = TTM_PL_FLAG_WC;
>> + break;
>> + default:
>> + DRM_ERROR("Unsupported memory type %u\n", type);
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> +void hibmc_ttm_placement(struct hibmc_bo *bo, int domain)
>> +{
>> + u32 c = 0;
>
> Can you please use a more descriptive name than 'c'?
ok, will do that.
>
>> + u32 i;
>> +
>> + bo->placement.placement = bo->placements;
>> + bo->placement.busy_placement = bo->placements;
>> + if (domain & TTM_PL_FLAG_VRAM)
>> + bo->placements[c++].flags = TTM_PL_FLAG_WC |
>> + TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM;
>
> nit: you're alignment is off here and below
is it correct?
if (domain & TTM_PL_FLAG_VRAM)
bo->placements[c++].flags = TTM_PL_FLAG_WC |
TTM_PL_FLAG_UNCACHED | TTM_PL_FLAG_VRAM;
if (domain & TTM_PL_FLAG_SYSTEM)
bo->placements[c++].flags = TTM_PL_MASK_CACHING |
TTM_PL_FLAG_SYSTEM;
if (!c)
bo->placements[c++].flags = TTM_PL_MASK_CACHING |
TTM_PL_FLAG_SYSTEM;
>
>> + if (domain & TTM_PL_FLAG_SYSTEM)
>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING |
>> + TTM_PL_FLAG_SYSTEM;
>> + if (!c)
>> + bo->placements[c++].flags = TTM_PL_MASK_CACHING |
>> + TTM_PL_FLAG_SYSTEM;
>> +
>> + bo->placement.num_placement = c;
>> + bo->placement.num_busy_placement = c;
>> + for (i = 0; i < c; ++i) {
>
> nit: we tend towards post-increment in kernel
agreed, thanks.
>
>> + bo->placements[i].fpfn = 0;
>> + bo->placements[i].lpfn = 0;
>> + }
>> +}
>> +
>> +static void
>> +hibmc_bo_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl)
>> +{
>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo);
>> +
>> + if (!hibmc_ttm_bo_is_hibmc_bo(bo))
>> + return;
>> +
>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_SYSTEM);
>> + *pl = hibmcbo->placement;
>> +}
>> +
>> +static int hibmc_bo_verify_access(struct ttm_buffer_object *bo,
>> + struct file *filp)
>> +{
>> + struct hibmc_bo *hibmcbo = hibmc_bo(bo);
>> +
>> + return drm_vma_node_verify_access(&hibmcbo->gem.vma_node,
>> + filp->private_data);
>> +}
>> +
>> +static int hibmc_ttm_io_mem_reserve(struct ttm_bo_device *bdev,
>> + struct ttm_mem_reg *mem)
>> +{
>> + struct ttm_mem_type_manager *man = &bdev->man[mem->mem_type];
>> + struct hibmc_drm_device *hibmc = hibmc_bdev(bdev);
>> +
>> + mem->bus.addr = NULL;
>> + mem->bus.offset = 0;
>> + mem->bus.size = mem->num_pages << PAGE_SHIFT;
>> + mem->bus.base = 0;
>> + mem->bus.is_iomem = false;
>> + if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE))
>> + return -EINVAL;
>> + switch (mem->mem_type) {
>> + case TTM_PL_SYSTEM:
>> + /* system memory */
>> + return 0;
>> + case TTM_PL_VRAM:
>> + mem->bus.offset = mem->start << PAGE_SHIFT;
>> + mem->bus.base = pci_resource_start(hibmc->dev->pdev, 0);
>> + mem->bus.is_iomem = true;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + return 0;
>> +}
>> +
>> +static void hibmc_ttm_io_mem_free(struct ttm_bo_device *bdev,
>> + struct ttm_mem_reg *mem)
>> +{
>> +}
>
> No need to stub this, the caller does a NULL-check before invoking
will delete it, thanks.
>
>> +
>> +static void hibmc_ttm_backend_destroy(struct ttm_tt *tt)
>> +{
>> + ttm_tt_fini(tt);
>> + kfree(tt);
>> +}
>> +
>> +static struct ttm_backend_func hibmc_tt_backend_func = {
>> + .destroy = &hibmc_ttm_backend_destroy,
>> +};
>> +
>> +static struct ttm_tt *hibmc_ttm_tt_create(struct ttm_bo_device *bdev,
>> + unsigned long size,
>> + u32 page_flags,
>> + struct page *dummy_read_page)
>> +{
>> + struct ttm_tt *tt;
>> +
>> + tt = kzalloc(sizeof(*tt), GFP_KERNEL);
>> + if (!tt)
>
> Print error
ok, will do that, thanks.
>
>> + return NULL;
>> + tt->func = &hibmc_tt_backend_func;
>> + if (ttm_tt_init(tt, bdev, size, page_flags, dummy_read_page)) {
>
> Here too?
ditto
>
>> + kfree(tt);
>> + return NULL;
>> + }
>> + return tt;
>> +}
>> +
>> +static int hibmc_ttm_tt_populate(struct ttm_tt *ttm)
>> +{
>> + return ttm_pool_populate(ttm);
>> +}
>> +
>> +static void hibmc_ttm_tt_unpopulate(struct ttm_tt *ttm)
>> +{
>> + ttm_pool_unpopulate(ttm);
>> +}
>> +
>> +struct ttm_bo_driver hibmc_bo_driver = {
>> + .ttm_tt_create = hibmc_ttm_tt_create,
>> + .ttm_tt_populate = hibmc_ttm_tt_populate,
>> + .ttm_tt_unpopulate = hibmc_ttm_tt_unpopulate,
>> + .init_mem_type = hibmc_bo_init_mem_type,
>> + .evict_flags = hibmc_bo_evict_flags,
>> + .move = NULL,
>> + .verify_access = hibmc_bo_verify_access,
>> + .io_mem_reserve = &hibmc_ttm_io_mem_reserve,
>> + .io_mem_free = &hibmc_ttm_io_mem_free,
>> + .lru_tail = &ttm_bo_default_lru_tail,
>> + .swap_lru_tail = &ttm_bo_default_swap_lru_tail,
>> +};
>> +
>> +int hibmc_mm_init(struct hibmc_drm_device *hibmc)
>> +{
>> + int ret;
>> + struct drm_device *dev = hibmc->dev;
>> + struct ttm_bo_device *bdev = &hibmc->ttm.bdev;
>> +
>> + ret = hibmc_ttm_global_init(hibmc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = ttm_bo_device_init(&hibmc->ttm.bdev,
>> + hibmc->ttm.bo_global_ref.ref.object,
>> + &hibmc_bo_driver,
>> + dev->anon_inode->i_mapping,
>> + DRM_FILE_PAGE_OFFSET,
>> + true);
>> + if (ret) {
>
> Call hibmc_ttm_global_release here?
agreed, thanks for pointing it out.
>
>> + DRM_ERROR("Error initialising bo driver; %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = ttm_bo_init_mm(bdev, TTM_PL_VRAM,
>> + hibmc->fb_size >> PAGE_SHIFT);
>> + if (ret) {
>
> Clean up here as well?
ditto
>
>> + DRM_ERROR("Failed ttm VRAM init: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + hibmc->mm_inited = true;
>> + return 0;
>> +}
>> +
>> +void hibmc_mm_fini(struct hibmc_drm_device *hibmc)
>> +{
>> + if (!hibmc->mm_inited)
>> + return;
>> +
>> + ttm_bo_device_release(&hibmc->ttm.bdev);
>> + hibmc_ttm_global_release(hibmc);
>> + hibmc->mm_inited = false;
>> +}
>> +
>> +int hibmc_bo_create(struct drm_device *dev, int size, int align,
>> + u32 flags, struct hibmc_bo **phibmcbo)
>> +{
>> + struct hibmc_drm_device *hibmc = dev->dev_private;
>> + struct hibmc_bo *hibmcbo;
>> + size_t acc_size;
>> + int ret;
>> +
>> + hibmcbo = kzalloc(sizeof(*hibmcbo), GFP_KERNEL);
>> + if (!hibmcbo)
>> + return -ENOMEM;
>> +
>> + ret = drm_gem_object_init(dev, &hibmcbo->gem, size);
>> + if (ret) {
>> + kfree(hibmcbo);
>> + return ret;
>> + }
>> +
>> + hibmcbo->bo.bdev = &hibmc->ttm.bdev;
>> +
>> + hibmc_ttm_placement(hibmcbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
>> +
>> + acc_size = ttm_bo_dma_acc_size(&hibmc->ttm.bdev, size,
>> + sizeof(struct hibmc_bo));
>> +
>> + ret = ttm_bo_init(&hibmc->ttm.bdev, &hibmcbo->bo, size,
>> + ttm_bo_type_device, &hibmcbo->placement,
>> + align >> PAGE_SHIFT, false, NULL, acc_size,
>> + NULL, NULL, hibmc_bo_ttm_destroy);
>> + if (ret)
>
> Missing hibmcbo clean up here
i looked at all other ttm drivers and all of them return directly when ttm_bo_init
failed, however, i think it is better to clean up here, should i call
hibmc_bo_unref(&hibmc_bo) here ?
>
>> + return ret;
>> +
>> + *phibmcbo = hibmcbo;
>> + return 0;
>> +}
>> +
>> +static inline u64 hibmc_bo_gpu_offset(struct hibmc_bo *bo)
>> +{
>> + return bo->bo.offset;
>> +}
>
> I don't think this function provides any value
do you nean i use bo->bo.offset instead of calling hibmc_bo_gpu_offset()?
>
>> +
>> +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr)
>> +{
>> + int i, ret;
>> +
>> + if (bo->pin_count) {
>> + bo->pin_count++;
>> + if (gpu_addr)
>> + *gpu_addr = hibmc_bo_gpu_offset(bo);
>
> Are you missing a return here?
Thanks for pointing it out!
>
>> + }
>> +
>> + hibmc_ttm_placement(bo, pl_flag);
>> + for (i = 0; i < bo->placement.num_placement; i++)
>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + bo->pin_count = 1;
>> + if (gpu_addr)
>> + *gpu_addr = hibmc_bo_gpu_offset(bo);
>> + return 0;
>> +}
>> +
>> +int hibmc_bo_push_sysram(struct hibmc_bo *bo)
>> +{
>> + int i, ret;
>> +
>> + if (!bo->pin_count) {
>> + DRM_ERROR("unpin bad %p\n", bo);
>> + return 0;
>> + }
>> + bo->pin_count--;
>> + if (bo->pin_count)
>> + return 0;
>> +
>> + if (bo->kmap.virtual)
>
> ttm_bo_kunmap already does this check so you don't have to
agreed. will remove this condition.
>
>> + ttm_bo_kunmap(&bo->kmap);
>> +
>> + hibmc_ttm_placement(bo, TTM_PL_FLAG_SYSTEM);
>> + for (i = 0; i < bo->placement.num_placement ; i++)
>> + bo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
>> +
>> + ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
>> + if (ret) {
>> + DRM_ERROR("pushing to VRAM failed\n");
>
> Print ret
ok, thanks.
>
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma)
>> +{
>> + struct drm_file *file_priv;
>> + struct hibmc_drm_device *hibmc;
>> +
>> + if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET))
>> + return -EINVAL;
>> +
>> + file_priv = filp->private_data;
>> + hibmc = file_priv->minor->dev->dev_private;
>> + return ttm_bo_mmap(filp, vma, &hibmc->ttm.bdev);
>> +}
>> +
>> +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel,
>> + struct drm_gem_object **obj)
>> +{
>> + struct hibmc_bo *hibmcbo;
>> + int ret;
>> +
>> + *obj = NULL;
>> +
>> + size = PAGE_ALIGN(size);
>> + if (size == 0)
>
> Print error
ditto
>
>> + return -EINVAL;
>> +
>> + ret = hibmc_bo_create(dev, size, 0, 0, &hibmcbo);
>> + if (ret) {
>> + if (ret != -ERESTARTSYS)
>> + DRM_ERROR("failed to allocate GEM object\n");
>
> Print ret
ditto
>
>> + return ret;
>> + }
>> + *obj = &hibmcbo->gem;
>> + return 0;
>> +}
>> +
>> +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev,
>> + struct drm_mode_create_dumb *args)
>> +{
>> + struct drm_gem_object *gobj;
>> + u32 handle;
>> + int ret;
>> +
>> + args->pitch = ALIGN(args->width * ((args->bpp + 7) / 8), 16);
>
> What's up with the bpp + 7 here? Perhaps you're looking for DIV_ROUND_UP?
Yes, that sounds sane.
>
>
>> + args->size = args->pitch * args->height;
>> +
>> + ret = hibmc_gem_create(dev, args->size, false,
>> + &gobj);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_gem_handle_create(file, gobj, &handle);
>> + drm_gem_object_unreference_unlocked(gobj);
>> + if (ret)
>
> Print error here
agreed.
>
>> + return ret;
>> +
>> + args->handle = handle;
>> + return 0;
>> +}
>> +
>> +static void hibmc_bo_unref(struct hibmc_bo **bo)
>> +{
>> + struct ttm_buffer_object *tbo;
>> +
>> + if ((*bo) == NULL)
>> + return;
>> +
>> + tbo = &((*bo)->bo);
>> + ttm_bo_unref(&tbo);
>> + *bo = NULL;
>> +}
>> +
>> +void hibmc_gem_free_object(struct drm_gem_object *obj)
>> +{
>> + struct hibmc_bo *hibmcbo = gem_to_hibmc_bo(obj);
>> +
>> + hibmc_bo_unref(&hibmcbo);
>> +}
>> +
>> +static u64 hibmc_bo_mmap_offset(struct hibmc_bo *bo)
>> +{
>> + return drm_vma_node_offset_addr(&bo->bo.vma_node);
>> +}
>> +
>> +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev,
>> + u32 handle, u64 *offset)
>> +{
>> + struct drm_gem_object *obj;
>> + struct hibmc_bo *bo;
>> +
>> + obj = drm_gem_object_lookup(file, handle);
>> + if (!obj)
>> + return -ENOENT;
>> +
>> + bo = gem_to_hibmc_bo(obj);
>> + *offset = hibmc_bo_mmap_offset(bo);
>> +
>> + drm_gem_object_unreference_unlocked(obj);
>> + return 0;
>> +}
Regards,
Rongrong.
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> _______________________________________________
> linuxarm mailing list
> linuxarm at huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>
> .
>
^ permalink raw reply
* [PATCH v5 03/23] usb: ulpi: Support device discovery via DT
From: Heikki Krogerus @ 2016-11-11 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161018015636.11701-4-stephen.boyd@linaro.org>
On Mon, Oct 17, 2016 at 06:56:16PM -0700, Stephen Boyd wrote:
> The qcom HSIC ULPI phy doesn't have any bits set in the vendor or
> product ID registers. This makes it impossible to make a ULPI
> driver match against the ID registers. Add support to discover
> the ULPI phys via DT help alleviate this problem. In the DT case,
> we'll look for a ULPI bus node underneath the device registering
> the ULPI viewport (or the parent of that device to support
> chipidea's device layout) and then match up the phy node
> underneath that with the ULPI device that's created.
>
> The side benefit of this is that we can use standard properties
> in the phy node like clks, regulators, gpios, etc. because we
> don't have firmware like ACPI to turn these things on for us. And
> we can use the DT phy binding to point our phy consumer to the
> phy provider.
>
> The ULPI bus code supports native enumeration by reading the
> vendor ID and product ID registers at device creation time, but
> we can't be certain that those register reads will succeed if the
> phy is not powered up. To avoid any problems with reading the ID
> registers before the phy is powered we fallback to DT matching
> when the ID reads fail.
>
> If the ULPI spec had some generic power sequencing for these
> registers we could put that into the ULPI bus layer and power up
> the device before reading the ID registers. Unfortunately this
> doesn't exist and the power sequence is usually device specific.
> By having the device matched up with DT we can avoid this
> problem.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: <devicetree@vger.kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>
Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
--
heikki
^ permalink raw reply
* [PATCH] ARM: davinci: enable PM for DT boot
From: Sekhar Nori @ 2016-11-11 11:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <m2shr16dr7.fsf@baylibre.com>
On Tuesday 08 November 2016 11:43 PM, Kevin Hilman wrote:
> Hi Sekhar,
>
> Sekhar Nori <nsekhar@ti.com> writes:
>
>> On Wednesday 26 October 2016 03:17 AM, Kevin Hilman wrote:
>>> Currently system PM is only enabled for legacy (non-DT) boot. Enable
>>> for DT boot also.
>>>
>>> Tested on da850-lcdk using "rtcwake -m mem -s5 -d rtc0".
>>>
>>> Signed-off-by: Kevin Hilman <khilman@baylibre.com>
>>> ---
>>> arch/arm/mach-davinci/da8xx-dt.c | 18 ++++++++++++++++++
>>> 1 file changed, 18 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-davinci/da8xx-dt.c b/arch/arm/mach-davinci/da8xx-dt.c
>>> index c9f7e9274aa8..a8089fa40d86 100644
>>> --- a/arch/arm/mach-davinci/da8xx-dt.c
>>> +++ b/arch/arm/mach-davinci/da8xx-dt.c
>>> @@ -43,8 +43,26 @@ static struct of_dev_auxdata da850_auxdata_lookup[] __initdata = {
>>>
>>> #ifdef CONFIG_ARCH_DAVINCI_DA850
>>>
>>> +static struct davinci_pm_config da850_pm_pdata = {
>>> + .sleepcount = 128,
>>> +};
>>> +
>>> +static struct platform_device da850_pm_device = {
>>> + .name = "pm-davinci",
>>> + .dev = {
>>> + .platform_data = &da850_pm_pdata,
>>> + },
>>> + .id = -1,
>>> +};
>>> +
>>> static void __init da850_init_machine(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = da850_register_pm(&da850_pm_device);
>>
>> I am not sure if it makes sense to keep the "pm device" around anymore.
>> I think for both DT and non-DT boot, we can get rid of the fake PM
>> device and combine da850_register_pm() and davinci_pm_probe() into a
>> single davinci_init_suspend() function which can then be called both for
>> DT and non-DT boot.
>
> Looking closer at this, where do you propose the pdata comes from for
> the non-DT boot?
>
> It seems to me that we can't currently remove the pdata dependency
> without breaking the non-DT platforms, so the approach proposed here is
> the least invasive.
There is a single value of sleep count that is used today (128). So I
was thinking we can hardcode that in pm.c. We are not going to add more
board files anyway so there is no risk here.
For future, if a different sleepcount value is needed, it will need to
be a new DT property.
Thanks,
Sekhar
^ permalink raw reply
* [PATCH] [media] mtk-mdp: allocate video_device dynamically
From: Hans Verkuil @ 2016-11-11 10:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478522529-57129-2-git-send-email-minghsiu.tsai@mediatek.com>
Almost correct:
On 11/07/2016 01:42 PM, Minghsiu Tsai wrote:
> It can fix known problems with embedded video_device structs.
>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
> drivers/media/platform/mtk-mdp/mtk_mdp_core.h | 2 +-
> drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 33 ++++++++++++++++-----------
> 2 files changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> index 848569d..ad1cff3 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_core.h
> @@ -167,7 +167,7 @@ struct mtk_mdp_dev {
> struct mtk_mdp_comp *comp[MTK_MDP_COMP_ID_MAX];
> struct v4l2_m2m_dev *m2m_dev;
> struct list_head ctx_list;
> - struct video_device vdev;
> + struct video_device *vdev;
> struct v4l2_device v4l2_dev;
> struct workqueue_struct *job_wq;
> struct platform_device *vpu_dev;
> diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> index 9a747e7..b8dee1c 100644
> --- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> +++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
> @@ -1236,16 +1236,22 @@ int mtk_mdp_register_m2m_device(struct mtk_mdp_dev *mdp)
> int ret;
>
> mdp->variant = &mtk_mdp_default_variant;
> - mdp->vdev.device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> - mdp->vdev.fops = &mtk_mdp_m2m_fops;
> - mdp->vdev.ioctl_ops = &mtk_mdp_m2m_ioctl_ops;
> - mdp->vdev.release = video_device_release_empty;
> - mdp->vdev.lock = &mdp->lock;
> - mdp->vdev.vfl_dir = VFL_DIR_M2M;
> - mdp->vdev.v4l2_dev = &mdp->v4l2_dev;
> - snprintf(mdp->vdev.name, sizeof(mdp->vdev.name), "%s:m2m",
> + mdp->vdev = video_device_alloc();
> + if (!mdp->vdev) {
> + dev_err(dev, "failed to allocate video device\n");
> + ret = -ENOMEM;
> + goto err_video_alloc;
> + }
> + mdp->vdev->device_caps = V4L2_CAP_VIDEO_M2M_MPLANE | V4L2_CAP_STREAMING;
> + mdp->vdev->fops = &mtk_mdp_m2m_fops;
> + mdp->vdev->ioctl_ops = &mtk_mdp_m2m_ioctl_ops;
> + mdp->vdev->release = video_device_release;
> + mdp->vdev->lock = &mdp->lock;
> + mdp->vdev->vfl_dir = VFL_DIR_M2M;
> + mdp->vdev->v4l2_dev = &mdp->v4l2_dev;
> + snprintf(mdp->vdev->name, sizeof(mdp->vdev->name), "%s:m2m",
> MTK_MDP_MODULE_NAME);
> - video_set_drvdata(&mdp->vdev, mdp);
> + video_set_drvdata(mdp->vdev, mdp);
>
> mdp->m2m_dev = v4l2_m2m_init(&mtk_mdp_m2m_ops);
> if (IS_ERR(mdp->m2m_dev)) {
> @@ -1254,26 +1260,27 @@ int mtk_mdp_register_m2m_device(struct mtk_mdp_dev *mdp)
> goto err_m2m_init;
> }
>
> - ret = video_register_device(&mdp->vdev, VFL_TYPE_GRABBER, 2);
> + ret = video_register_device(mdp->vdev, VFL_TYPE_GRABBER, 2);
> if (ret) {
> dev_err(dev, "failed to register video device\n");
> goto err_vdev_register;
> }
>
> v4l2_info(&mdp->v4l2_dev, "driver registered as /dev/video%d",
> - mdp->vdev.num);
> + mdp->vdev->num);
> return 0;
>
> err_vdev_register:
> v4l2_m2m_release(mdp->m2m_dev);
> err_m2m_init:
> - video_device_release(&mdp->vdev);
> + video_unregister_device(mdp->vdev);
This should remain video_device_release: the video_register_device call failed, so
the device hasn't been registered. In that case just release the device (i.e.
free the allocated memory).
> +err_video_alloc:
>
> return ret;
> }
>
> void mtk_mdp_unregister_m2m_device(struct mtk_mdp_dev *mdp)
> {
> - video_device_release(&mdp->vdev);
> + video_unregister_device(mdp->vdev);
> v4l2_m2m_release(mdp->m2m_dev);
> }
>
Regards,
Hans
^ permalink raw reply
* [PATCH V5 3/3] ARM64 LPC: LPC driver implementation on Hip06
From: liviu.dudau at arm.com @ 2016-11-11 10:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <10334260.ztLXZ2Oynd@wuerfel>
Hi Arnd,
On Thu, Nov 10, 2016 at 05:07:21PM +0100, Arnd Bergmann wrote:
> On Thursday, November 10, 2016 3:36:49 PM CET Gabriele Paoloni wrote:
> >
> > Where should we get the range from? For LPC we know that it is going
> > Work on anything that is not used by PCI I/O space, and this is
> > why we use [0, PCIBIOS_MIN_IO]
>
> It should be allocated the same way we allocate PCI config space
> segments. This is currently done with the io_range list in
> drivers/pci/pci.c, which isn't perfect but could be extended
> if necessary. Based on what others commented here, I'd rather
> make the differences between ISA/LPC and PCI I/O ranges smaller
> than larger.
>
> > > Your current version has
> > >
> > > if (arm64_extio_ops->pfout) \
> > > arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > addr, value, sizeof(type)); \
> > >
> > > Instead, just subtract the start of the range from the logical
> > > port number to transform it back into a bus-local port number:
> >
> > These accessors do not operate on IO tokens:
> >
> > If (arm64_extio_ops->start > addr || arm64_extio_ops->end < addr)
> > addr is not going to be an I/O token; in fact patch 2/3 imposes that
> > the I/O tokens will start at PCIBIOS_MIN_IO. So from 0 to PCIBIOS_MIN_IO
> > we have free physical addresses that the accessors can operate on.
>
> Ah, I missed that part. I'd rather not use PCIBIOS_MIN_IO to refer to
> the logical I/O tokens, the purpose of that macro is really meant
> for allocating PCI I/O port numbers within the address space of
> one bus.
>
> Note that it's equally likely that whichever next platform needs
> non-mapped I/O access like this actually needs them for PCI I/O space,
> and that will use it on addresses registered to a PCI host bridge.
>
> If we separate the two steps:
>
> a) assign a range of logical I/O port numbers to a bus
Except that currently when we add ranges to io_range_list we don't have
a bus number yet, because the parsing happens before the host bridge
has been created. Maybe register_io_range() can take a bus number as an
argument, but I'm not sure how we are going to use that in pci_pio_to_address()
or pci_address_to_pio().
Best regards,
Liviu
> b) register a set of helpers for redirecting logical I/O
> port to a helper function
>
> then I think the code will get cleaner and more flexible.
> It should actually then be able to replace the powerpc
> specific implementation.
>
> Arnd
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v2 2/5] ARM: bus: da8xx-mstpri: new driver
From: Sekhar Nori @ 2016-11-11 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109182433.vskk5rn7hfxshfy6@rob-hp-laptop>
On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:35PM +0100, Bartosz Golaszewski wrote:
>> Create the driver for the da8xx master peripheral priority
>> configuration and implement support for writing to the three
>> Master Priority registers on da850 SoCs.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> .../devicetree/bindings/bus/ti,da850-mstpri.txt | 20 ++
>
> Acked-by: Rob Herring <robh@kernel.org>
Applied to v4.10/drivers
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v2 1/5] ARM: memory: da8xx-ddrctl: new driver
From: Sekhar Nori @ 2016-11-11 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161109182430.yh6uqeu2ufzkctww@rob-hp-laptop>
On Wednesday 09 November 2016 11:54 PM, Rob Herring wrote:
> On Mon, Oct 31, 2016 at 03:45:34PM +0100, Bartosz Golaszewski wrote:
>> Create a new driver for the da8xx DDR2/mDDR controller and implement
>> support for writing to the Peripheral Bus Burst Priority Register.
>>
>> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>> ---
>> .../memory-controllers/ti-da8xx-ddrctl.txt | 20 +++
>
> Acked-by: Rob Herring <robh@kernel.org>
Applied to v4.10/drivers
Thanks,
Sekhar
^ permalink raw reply
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Jan Glauber @ 2016-11-11 10:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161110165405.GH4418@leverpostej>
Hi Mark,
thanks for reviewing. One question below, for most of your other comments
I think we need to come to a conclusion about the aggregation first.
On Thu, Nov 10, 2016 at 04:54:06PM +0000, Mark Rutland wrote:
> Hi Jan,
>
> Apologies for the delay in getting to this.
>
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * Cavium Thunder uncore PMU support.
> > + *
> > + * Copyright (C) 2015,2016 Cavium Inc.
> > + * Author: Jan Glauber <jan.glauber@cavium.com>
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/numa.h>
> > +#include <linux/slab.h>
>
> I believe the following includes are necessary for APIs and/or data
> explicitly referenced by the driver code:
>
> #include <linux/atomic.h>
> #include <linux/cpuhotplug.h>
> #include <linux/cpumask.h>
> #include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/pci.h>
> #include <linux/perf_event.h>
> #include <linux/printk.h>
> #include <linux/smp.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
>
> #include <asm/local64.h>
>
> ... please add those here.
Should I also add includes that are already in the included by uncore_cavium.h?
I usually avoid includes that come through the "local" header file.
^ permalink raw reply
* [PATCH 0/6] mfd: audit and demodule drivers/mfd/ab* drivers
From: Lee Jones @ 2016-11-11 10:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161030012440.10495-1-paul.gortmaker@windriver.com>
On Sat, 29 Oct 2016, Paul Gortmaker wrote:
> Nothing new here ; just another instance where we had modular remove
> and exit functions -- but the Makefile/Kconfig limited the use case to
> being always built-in; hence the code is useless and needs removing.
>
> So we remove the dead code and the misleading hints of modularity from
> these drivers, hoping that we manage to prevent some new drivers from
> copying these same old mistakes into pending new drivers.
>
> Paul.
> ---
>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Mattias Wallin <mattias.wallin@stericsson.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: linux-arm-kernel at lists.infradead.org
>
> Paul Gortmaker (6):
> mfd: ab3100-core: Make it explicitly non-modular
> mfd: ab8500-core: Make it explicitly non-modular
> mfd: ab8500-debugfs: Make it explicitly non-modular
> mfd: ab8500-gpadc: Make it explicitly non-modular
> mfd: ab8500: make sysctrl explicitly non-modular
> mfd: abx500-core: drop unused MODULE_ tags from non-modular code
Applied the series with Linus' Ack.
> drivers/mfd/ab3100-core.c | 39 +++------------------------------------
> drivers/mfd/ab8500-core.c | 37 ++++++-------------------------------
> drivers/mfd/ab8500-debugfs.c | 21 ++-------------------
> drivers/mfd/ab8500-gpadc.c | 19 ++-----------------
> drivers/mfd/ab8500-sysctrl.c | 9 ++++-----
> drivers/mfd/abx500-core.c | 7 ++-----
> 6 files changed, 19 insertions(+), 113 deletions(-)
>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH V5 2/3] ARM64 LPC: Add missing range exception for special ISA
From: liviu.dudau at arm.com @ 2016-11-11 10:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E1F8F5F06@lhreml507-mbx>
On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
>
> > -----Original Message-----
> > From: liviu.dudau at arm.com [mailto:liviu.dudau at arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.marinas at arm.com; will.deacon at arm.com;
> > robh+dt at kernel.org; bhelgaas at google.com; mark.rutland at arm.com;
> > olof at lixom.net; arnd at arndb.de; linux-arm-kernel at lists.infradead.org;
> > lorenzo.pieralisi at arm.com; linux-kernel at vger.kernel.org; Linuxarm;
> > devicetree at vger.kernel.org; linux-pci at vger.kernel.org; linux-
> > serial at vger.kernel.org; minyard at acm.org; benh at kernel.crashing.org;
> > zourongrong at gmail.com; John Garry; zhichang.yuan02 at gmail.com;
> > kantyzc at 163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> >
> > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> >
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> >
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> >
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> >
> > >
> > > >
> > > > > + return 0;
> > > > > +
> > > > > + return 1;
> > > > > +}
> > > > >
> > > > > BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > > return false;
> > > > > }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + * For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + * The device addresses are described directly in their regs
> > > > property.
> > > > > + * This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + * devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent: points to the parent dts node;
> > > > > + * @bus: points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr: the address from reg property;
> > > > > + * @na: the address cell counter of @addr;
> > > > > + * @presult: store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > + struct of_bus *bus, __be32 *addr,
> > > > > + int na, u64 *presult)
> > > > > +{
> > > > > + unsigned int flags;
> > > > > + unsigned int rlen;
> > > > > +
> > > > > + /* whether support indirectIO */
> > > > > + if (!indirect_io_enabled())
> > > > > + return 0;
> > > > > +
> > > > > + if (!of_bus_isa_match(parent))
> > > > > + return 0;
> > > > > +
> > > > > + flags = bus->get_flags(addr);
> > > > > + if (!(flags & IORESOURCE_IO))
> > > > > + return 0;
> > > > > +
> > > > > + /* there is ranges property, apply the normal translation
> > > > directly. */
> > > >
> > > > s/there is ranges/if we have a 'ranges'/
> > >
> > > Thanks for spotting this
> > >
> > > >
> > > > > + if (of_get_property(parent, "ranges", &rlen))
> > > > > + return 0;
> > > > > +
> > > > > + *presult = of_read_number(addr + 1, na - 1);
> > > > > + /* this fixup is only valid for specific I/O range. */
> > > > > + return addr_is_indirect_io(*presult);
> > > > > +}
> > > > > +
> > > > > static int of_translate_one(struct device_node *parent, struct
> > > > of_bus *bus,
> > > > > struct of_bus *pbus, __be32 *addr,
> > > > > int na, int ns, int pna, const char *rprop)
> > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > > device_node *dev,
> > > > > result = of_read_number(addr, na);
> > > > > break;
> > > > > }
> > > > > + /*
> > > > > + * For indirectIO device which has no ranges
> > property, get
> > > > > + * the address from reg directly.
> > > > > + */
> > > > > + if (of_get_isa_indirect_io(dev, bus, addr, na,
> > &result)) {
> > > > > + pr_debug("isa indirectIO matched(%s)..addr =
> > > > 0x%llx\n",
> > > > > + of_node_full_name(dev), result);
> > > > > + break;
> > > > > + }
> > > > >
> > > > > /* Get new parent bus and counts */
> > > > > pbus = of_match_bus(parent);
> > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > > device_node *dev,
> > > > > if (taddr == OF_BAD_ADDR)
> > > > > return -EINVAL;
> > > > > memset(r, 0, sizeof(struct resource));
> > > > > - if (flags & IORESOURCE_IO) {
> > > > > + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > > > unsigned long port;
> > > > > +
> > > > > port = pci_address_to_pio(taddr);
> > > > > if (port == (unsigned long)-1)
> > > > > return -EINVAL;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index ba34907..1a08511 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -3263,7 +3263,7 @@ int __weak
> > pci_register_io_range(phys_addr_t
> > > > addr, resource_size_t size)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > > >
> > > > > /* check if the range hasn't been previously recorded */
> > > > > spin_lock(&io_range_lock);
> > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> > long
> > > > pio)
> > > > >
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *range;
> > > > > - resource_size_t allocated_size = 0;
> > > > > + resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > > Have you checked that pci_pio_to_address still returns valid values
> > > > after this? I know that
> > > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > > allocating reserving the IO ranges,
> > > > but the values added in the io_range_list are still starting from
> > zero,
> > > > no from PCIBIOS_MIN_IO,
> > >
> > > I think you're wrong here as in pci_address_to_pio we have:
> > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > >
> > > This should be enough to guarantee that the PIOs start at
> > > PCIBIOS_MIN_IO...right?
> >
> > I don't think you can guarantee that the pio value that gets passed
> > into
> > pci_pio_to_address() always comes from a previously returned value by
> > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
>
> Maybe I am missing something...could you make an exampleof a case
> where an IO toke doesn?t come from pci_address_to_pio() ?
Don't know, maybe it is coming from some DT or platform data? I was just
asking for confirmation that no one has seen issues there, I'm not saying
I've seen it happening.
Best regards,
Liviu
>
> Thanks
>
> Gab
>
>
> >
> > if (pio < PCIBIOS_MIN_IO)
> > return address;
> >
> > to avoid adding more checks in the list_for_each_entry() loop.
> >
> > Best regards,
> > Liviu
> >
> > >
> > >
> > > > so the calculation of the address in this function could return
> > > > negative values casted to pci_addr_t.
> > > >
> > > > Maybe you want to adjust the range->start value in
> > > > pci_register_io_range() as well to have it
> > > > offset by PCIBIOS_MIN_IO as well.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > > if (pio > IO_SPACE_LIMIT)
> > > > > return address;
> > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > > pci_address_to_pio(phys_addr_t address)
> > > > > {
> > > > > #ifdef PCI_IOBASE
> > > > > struct io_range *res;
> > > > > - resource_size_t offset = 0;
> > > > > + resource_size_t offset = PCIBIOS_MIN_IO;
> > > > > unsigned long addr = -1;
> > > > >
> > > > > spin_lock(&io_range_lock);
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index 3786473..deec469 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > > > #define for_each_of_pci_range(parser, range) \
> > > > > for (; of_pci_range_parser_one(parser, range);)
> > > > >
> > > > > +
> > > > > +#ifndef indirect_io_enabled
> > > > > +#define indirect_io_enabled indirect_io_enabled
> > > > > +static inline bool indirect_io_enabled(void)
> > > > > +{
> > > > > + return false;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#ifndef addr_is_indirect_io
> > > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > + return 0;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > /* Translate a DMA address from device space to CPU space */
> > > > > extern u64 of_translate_dma_address(struct device_node *dev,
> > > > > const __be32 *in_addr);
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 0e49f70..7f6bbb6 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > > pci_bus *bus)
> > > > > /* provide the legacy pci_dma_* API */
> > > > > #include <linux/pci-dma-compat.h>
> > > > >
> > > > > +/*
> > > > > + * define this macro here to refrain from compilation error for
> > some
> > > > > + * platforms. Please keep this macro at the end of this header
> > file.
> > > > > + */
> > > > > +#ifndef PCIBIOS_MIN_IO
> > > > > +#define PCIBIOS_MIN_IO 0
> > > > > +#endif
> > > > > +
> > > > > #endif /* LINUX_PCI_H */
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-
> > pci"
> > > > in
> > > > > the body of a message to majordomo at vger.kernel.org
> > > > > More majordomo info at http://vger.kernel.org/majordomo-
> > info.html
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world, |
> > | but they're not |
> > | giving me the |
> > \ source code! /
> > ---------------
> > ?\_(?)_/?
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
From: Anurup M @ 2016-11-11 10:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <d009dcdc-2258-ed6a-53be-2211c7493051@huawei.com>
On Wednesday 09 November 2016 02:36 PM, John Garry wrote:
>
>>>>> I'd suggest requiring #address-cells=<1> and #size-cells=<0> in the
>>>>> master
>>>>> node, and listing the children by reg property. If the address is not
>>>>> easily expressed as a single integer, use a larger #address-cells
>>>>> value.
>>>> We already have something equivalent to reg in "module-id" (see patch
>>>> 02/11), which is the slave device bus address; here's a sample:
>>>> + /* For L3 cache PMU */
>>>> + pmul3c0 {
>>>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>>>> + scl-id = <0x02>;
>>>> + num-events = <0x16>;
>>>> + num-counters = <0x08>;
>>>> + module-id = <0x04>;
>>>> + num-banks = <0x04>;
>>>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>>>> + counter-reg = <0x170>;
>>>> + evctrl-reg = <0x04>;
>>>> + event-en = <0x1000000>;
>>>> + evtype-reg = <0x140>;
>>>> + };
>>>>
>>>> FYI, "module-id" is our own internal hw nomenclature.
>>> Yes, that was my interpretation as well. Please use the standard
>>> "reg" property for this then.
>> Hi Arnd,
>>
>> Firstly my apologies for a mistake in the bindings example in ([PATCH
>> 02/11 ..]).
>> The module-id property is a list as defined in the PMU bindings patch
>> ([PATCH v1 05/11] dt-bindings .. <https://lkml.org/lkml/2016/11/2/323>).
>>
>> + djtag0: djtag at 0 {
>> + compatible = "hisilicon,hip05-cpu-djtag-v1";
>> + pmul3c0 {
>> + compatible = "hisilicon,hisi-pmu-l3c-v1";
>> + scl-id = <0x02>;
>> + num-events = <0x16>;
>> + num-counters = <0x08>;
>> + module-id = <0x04 0x04 0x04 0x04>;
>> + num-banks = <0x04>;
>> + cfgen-map = <0x02 0x04 0x01 0x08>;
>> + counter-reg = <0x170>;
>> + evctrl-reg = <0x04>;
>> + event-en = <0x1000000>;
>> + evtype-reg = <0x140>;
>> + };
>>
>>
>> The L3 cache in hip05/06/07 chips consist of 4 banks (each bank has PMU
>> registers).
>>
>> In hip05/06 all L3 cache banks are identified with same module-id.
>> module-id = <0x04 0x04 0x04 0x04>;
>>
>> But in the case hip07 chip(djtag v2), each L3 cache bank has different
>> module-id
>> module-id = <0x01 0x02 0x03 0x04>;
>>
>> So in this case Please share your opinion on how to model it.
>>
>
> My suggestion is to have a single PMU per module, whether that is 4
> banks or 1 bank per module, as this makes the driver simpler.
>
> I think you mentioned that a separate PMU per bank does not make much
> sense, and you would rather treat all banks as a single bank and
> aggregrate their perf statstics under a single PMU: Can you just use a
> script in userspace which can do this aggregration work if you have
> separate PMUs?
Hi John,
Mark also suggest the same view.
I have some concerns or doubts in having separate PMU for each L3 cache
bank.
We can discuss it in the same thread [[RESEND PATCH v1 07/11]]
<http://www.spinics.net/lists/arm-kernel/msg541938.html>].
Thanks,
Anurup
>
> Maybe perf guys have a view on this also.
>
> John
>
>> Some more detail of L3 cache PMU.
>> ------------------------------------------------
>> The hip05/06/07 chips consists of a multiple Super CPU cluster (16 CPU
>> cores). we call it SCCL.
>> The L3 cache( 4 banks) is shared by all CPU cores in a SCCL.
>> Each L3 cache bank has PMU registers. We always take the sum of the
>> counters to show in perf.
>> Taking individual L3 cache count is not meaningful as there is no
>> mapping of CPU cores to individual
>> L3 cache banks.
>>
>> Please share your suggestion.
>>
>> Thanks,
>> Anurup
>
^ permalink raw reply
* [PATCH v4 1/5] arm64: perf: Basic uncore counter support for Cavium ThunderX SOC
From: Jan Glauber @ 2016-11-11 10:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161108235010.GC17771@arm.com>
Hi Will,
thanks for the review!
On Tue, Nov 08, 2016 at 11:50:10PM +0000, Will Deacon wrote:
> Hi Jan,
>
> Thanks for posting an updated series. I have a few minor comments, which
> we can hopefully address in time for 4.10.
>
> Also, have you run the perf fuzzer with this series applied?
No, that's new to me. Will try it.
> https://github.com/deater/perf_event_tests
>
> (build the tests and look under the fuzzer/ directory for the tool)
>
> On Sat, Oct 29, 2016 at 01:55:29PM +0200, Jan Glauber wrote:
> > Provide "uncore" facilities for different non-CPU performance
> > counter units.
> >
> > The uncore PMUs can be found under /sys/bus/event_source/devices.
> > All counters are exported via sysfs in the corresponding events
> > files under the PMU directory so the perf tool can list the event names.
> >
> > There are some points that are special in this implementation:
> >
> > 1) The PMU detection relies on PCI device detection. If a
> > matching PCI device is found the PMU is created. The code can deal
> > with multiple units of the same type, e.g. more than one memory
> > controller.
> >
> > 2) Counters are summarized across different units of the same type
> > on one NUMA node but not across NUMA nodes.
> > For instance L2C TAD 0..7 are presented as a single counter
> > (adding the values from TAD 0 to 7). Although losing the ability
> > to read a single value the merged values are easier to use.
> >
> > 3) The counters are not CPU related. A random CPU is picked regardless
> > of the NUMA node. There is a small performance penalty for accessing
> > counters on a remote note but reading a performance counter is a
> > slow operation anyway.
> >
> > Signed-off-by: Jan Glauber <jglauber@cavium.com>
> > ---
> > drivers/perf/Kconfig | 13 ++
> > drivers/perf/Makefile | 1 +
> > drivers/perf/uncore/Makefile | 1 +
> > drivers/perf/uncore/uncore_cavium.c | 351 ++++++++++++++++++++++++++++++++++++
> > drivers/perf/uncore/uncore_cavium.h | 71 ++++++++
>
> We already have "uncore" PMUs under drivers/perf, so I'd prefer that we
> renamed this a bit to reflect better what's going on. How about:
>
> drivers/perf/cavium/
>
> and then
>
> drivers/perf/cavium/uncore_thunder.[ch]
>
> ?
OK, agreed.
> > include/linux/cpuhotplug.h | 1 +
> > 6 files changed, 438 insertions(+)
> > create mode 100644 drivers/perf/uncore/Makefile
> > create mode 100644 drivers/perf/uncore/uncore_cavium.c
> > create mode 100644 drivers/perf/uncore/uncore_cavium.h
> >
> > diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> > index 4d5c5f9..3266c87 100644
> > --- a/drivers/perf/Kconfig
> > +++ b/drivers/perf/Kconfig
> > @@ -19,4 +19,17 @@ config XGENE_PMU
> > help
> > Say y if you want to use APM X-Gene SoC performance monitors.
> >
> > +config UNCORE_PMU
> > + bool
>
> This isn't needed.
I thought about a symbol for all uncore pmus. But when drivers/perf is
already that it can be removed.
> > +
> > +config UNCORE_PMU_CAVIUM
> > + depends on PERF_EVENTS && NUMA && ARM64
> > + bool "Cavium uncore PMU support"
>
> Please mentioned Thunder somewhere, since that's the SoC being supported.
OK.
> > + select UNCORE_PMU
> > + default y
> > + help
> > + Say y if you want to access performance counters of subsystems
> > + on a Cavium SOC like cache controller, memory controller or
> > + processor interconnect.
> > +
> > endmenu
> > diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> > index b116e98..d6c02c9 100644
> > --- a/drivers/perf/Makefile
> > +++ b/drivers/perf/Makefile
> > @@ -1,2 +1,3 @@
> > obj-$(CONFIG_ARM_PMU) += arm_pmu.o
> > obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> > +obj-y += uncore/
> > diff --git a/drivers/perf/uncore/Makefile b/drivers/perf/uncore/Makefile
> > new file mode 100644
> > index 0000000..6130e18
> > --- /dev/null
> > +++ b/drivers/perf/uncore/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_UNCORE_PMU_CAVIUM) += uncore_cavium.o
> > diff --git a/drivers/perf/uncore/uncore_cavium.c b/drivers/perf/uncore/uncore_cavium.c
> > new file mode 100644
> > index 0000000..a7b4277
> > --- /dev/null
> > +++ b/drivers/perf/uncore/uncore_cavium.c
> > @@ -0,0 +1,351 @@
> > +/*
> > + * Cavium Thunder uncore PMU support.
> > + *
> > + * Copyright (C) 2015,2016 Cavium Inc.
> > + * Author: Jan Glauber <jan.glauber@cavium.com>
> > + */
> > +
> > +#include <linux/cpufeature.h>
> > +#include <linux/numa.h>
> > +#include <linux/slab.h>
> > +
> > +#include "uncore_cavium.h"
> > +
> > +/*
> > + * Some notes about the various counters supported by this "uncore" PMU
> > + * and the design:
> > + *
> > + * All counters are 64 bit long.
> > + * There are no overflow interrupts.
> > + * Counters are summarized per node/socket.
> > + * Most devices appear as separate PCI devices per socket with the exception
> > + * of OCX TLK which appears as one PCI device per socket and contains several
> > + * units with counters that are merged.
> > + * Some counters are selected via a control register (L2C TAD) and read by
> > + * a number of counter registers, others (L2C CBC, LMC & OCX TLK) have
> > + * one dedicated counter per event.
> > + * Some counters are not stoppable (L2C CBC & LMC).
> > + * Some counters are read-only (LMC).
> > + * All counters belong to PCI devices, the devices may have additional
> > + * drivers but we assume we are the only user of the counter registers.
> > + * We map the whole PCI BAR so we must be careful to forbid access to
> > + * addresses that contain neither counters nor counter control registers.
> > + */
> > +
> > +void thunder_uncore_read(struct perf_event *event)
> > +{
>
> Rather than have a bunch of global symbols that are called from the
> individual drivers, why don't you pass a struct of function pointers to
> their respective init functions and keep the internals private?
Most of these functions are already in struct pmu. What I can do is
set the shared functions in thunder_uncore_setup as default, and
only override them as needed (like thunder_uncore_read_ocx_tlk)
or the other way around (use default if not set already).
Then I can get rid of them.
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 prev, delta, new = 0;
> > +
> > + node = get_node(hwc->config, uncore);
> > +
> > + /* read counter values from all units on the node */
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > +
> > + prev = local64_read(&hwc->prev_count);
> > + local64_set(&hwc->prev_count, new);
> > + delta = new - prev;
> > + local64_add(delta, &event->count);
> > +}
> > +
> > +int thunder_uncore_add(struct perf_event *event, int flags, u64 config_base,
> > + u64 event_base)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int id;
> > +
> > + node = get_node(hwc->config, uncore);
> > + id = get_id(hwc->config);
> > +
> > + if (!cmpxchg(&node->events[id], NULL, event))
> > + hwc->idx = id;
>
> Does this need to be a full-fat cmpxchg? Who are you racing with?
Just copy'n'paste from the existing drivers. I guess it can be relaxed.
> > +
> > + if (hwc->idx == -1)
> > + return -EBUSY;
>
> This would be much clearer as an else statement after the cmpxchg.
Agreed.
> > +
> > + hwc->config_base = config_base;
> > + hwc->event_base = event_base;
> > + hwc->state = PERF_HES_UPTODATE | PERF_HES_STOPPED;
> > +
> > + if (flags & PERF_EF_START)
> > + uncore->pmu.start(event, PERF_EF_RELOAD);
> > +
> > + return 0;
> > +}
> > +
> > +void thunder_uncore_del(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + int i;
> > +
> > + event->pmu->stop(event, PERF_EF_UPDATE);
> > +
> > + /*
> > + * For programmable counters we need to check where we installed it.
> > + * To keep this function generic always test the more complicated
> > + * case (free running counters won't need the loop).
> > + */
> > + node = get_node(hwc->config, uncore);
> > + for (i = 0; i < node->num_counters; i++) {
> > + if (cmpxchg(&node->events[i], event, NULL) == event)
> > + break;
> > + }
> > + hwc->idx = -1;
> > +}
> > +
> > +void thunder_uncore_start(struct perf_event *event, int flags)
> > +{
> > + struct thunder_uncore *uncore = to_uncore(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore_unit *unit;
> > + u64 new = 0;
> > +
> > + /* read counter values from all units on the node */
> > + node = get_node(hwc->config, uncore);
> > + list_for_each_entry(unit, &node->unit_list, entry)
> > + new += readq(hwc->event_base + unit->map);
> > + local64_set(&hwc->prev_count, new);
> > +
> > + hwc->state = 0;
> > + perf_event_update_userpage(event);
> > +}
> > +
> > +void thunder_uncore_stop(struct perf_event *event, int flags)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > +
> > + hwc->state |= PERF_HES_STOPPED;
> > +
> > + if ((flags & PERF_EF_UPDATE) && !(hwc->state & PERF_HES_UPTODATE)) {
> > + thunder_uncore_read(event);
> > + hwc->state |= PERF_HES_UPTODATE;
> > + }
> > +}
> > +
> > +int thunder_uncore_event_init(struct perf_event *event)
> > +{
> > + struct hw_perf_event *hwc = &event->hw;
> > + struct thunder_uncore_node *node;
> > + struct thunder_uncore *uncore;
> > +
> > + if (event->attr.type != event->pmu->type)
> > + return -ENOENT;
> > +
> > + /* we do not support sampling */
> > + if (is_sampling_event(event))
> > + return -EINVAL;
> > +
> > + /* counters do not have these bits */
> > + if (event->attr.exclude_user ||
> > + event->attr.exclude_kernel ||
> > + event->attr.exclude_host ||
> > + event->attr.exclude_guest ||
> > + event->attr.exclude_hv ||
> > + event->attr.exclude_idle)
> > + return -EINVAL;
> > +
> > + uncore = to_uncore(event->pmu);
> > + if (!uncore)
> > + return -ENODEV;
> > + if (!uncore->event_valid(event->attr.config & UNCORE_EVENT_ID_MASK))
> > + return -EINVAL;
> > +
> > + /* check NUMA node */
> > + node = get_node(event->attr.config, uncore);
> > + if (!node) {
> > + pr_debug("Invalid NUMA node selected\n");
> > + return -EINVAL;
> > + }
> > +
> > + hwc->config = event->attr.config;
> > + hwc->idx = -1;
> > + return 0;
> > +}
> > +
> > +static ssize_t thunder_uncore_attr_show_cpumask(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct pmu *pmu = dev_get_drvdata(dev);
> > + struct thunder_uncore *uncore =
> > + container_of(pmu, struct thunder_uncore, pmu);
> > +
> > + return cpumap_print_to_pagebuf(true, buf, &uncore->active_mask);
> > +}
> > +static DEVICE_ATTR(cpumask, S_IRUGO, thunder_uncore_attr_show_cpumask, NULL);
> > +
> > +static struct attribute *thunder_uncore_attrs[] = {
> > + &dev_attr_cpumask.attr,
> > + NULL,
> > +};
> > +
> > +struct attribute_group thunder_uncore_attr_group = {
> > + .attrs = thunder_uncore_attrs,
> > +};
> > +
> > +ssize_t thunder_events_sysfs_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *page)
> > +{
> > + struct perf_pmu_events_attr *pmu_attr =
> > + container_of(attr, struct perf_pmu_events_attr, attr);
> > +
> > + if (pmu_attr->event_str)
> > + return sprintf(page, "%s", pmu_attr->event_str);
> > +
> > + return 0;
> > +}
> > +
> > +/* node attribute depending on number of NUMA nodes */
> > +static ssize_t node_show(struct device *dev, struct device_attribute *attr,
> > + char *page)
> > +{
> > + if (NODES_SHIFT)
> > + return sprintf(page, "config:16-%d\n", 16 + NODES_SHIFT - 1);
>
> If NODES_SHIFT is 1, you'll end up with "config:16-16", which might confuse
> userspace.
So should I use "config:16" in that case? Is it OK to use this also for
NODES_SHIFT=0 ?
> > + else
> > + return sprintf(page, "config:16\n");
> > +}
> > +
> > +struct device_attribute format_attr_node = __ATTR_RO(node);
> > +
> > +/*
> > + * Thunder uncore events are independent from CPUs. Provide a cpumask
> > + * nevertheless to prevent perf from adding the event per-cpu and just
> > + * set the mask to one online CPU. Use the same cpumask for all uncore
> > + * devices.
> > + *
> > + * There is a performance penalty for accessing a device from a CPU on
> > + * another socket, but we do not care (yet).
> > + */
> > +static int thunder_uncore_offline_cpu(unsigned int old_cpu, struct hlist_node *node)
> > +{
> > + struct thunder_uncore *uncore = hlist_entry_safe(node, struct thunder_uncore, node);
>
> Why _safe?
Not required, will remove.
> > + int new_cpu;
> > +
> > + if (!cpumask_test_and_clear_cpu(old_cpu, &uncore->active_mask))
> > + return 0;
> > + new_cpu = cpumask_any_but(cpu_online_mask, old_cpu);
> > + if (new_cpu >= nr_cpu_ids)
> > + return 0;
> > + perf_pmu_migrate_context(&uncore->pmu, old_cpu, new_cpu);
> > + cpumask_set_cpu(new_cpu, &uncore->active_mask);
> > + return 0;
> > +}
> > +
> > +static struct thunder_uncore_node * __init alloc_node(struct thunder_uncore *uncore,
> > + int node_id, int counters)
> > +{
> > + struct thunder_uncore_node *node;
> > +
> > + node = kzalloc(sizeof(*node), GFP_KERNEL);
> > + if (!node)
> > + return NULL;
> > + node->num_counters = counters;
> > + INIT_LIST_HEAD(&node->unit_list);
> > + return node;
> > +}
> > +
> > +int __init thunder_uncore_setup(struct thunder_uncore *uncore, int device_id,
> > + struct pmu *pmu, int counters)
> > +{
> > + unsigned int vendor_id = PCI_VENDOR_ID_CAVIUM;
> > + struct thunder_uncore_unit *unit, *tmp;
> > + struct thunder_uncore_node *node;
> > + struct pci_dev *pdev = NULL;
> > + int ret, node_id, found = 0;
> > +
> > + /* detect PCI devices */
> > + while ((pdev = pci_get_device(vendor_id, device_id, pdev))) {
>
> Redundant brackets?
OK
> > + if (!pdev)
> > + break;
>
> Redundant check?
OK
> > + node_id = dev_to_node(&pdev->dev);
> > +
> > + /* allocate node if necessary */
> > + if (!uncore->nodes[node_id])
> > + uncore->nodes[node_id] = alloc_node(uncore, node_id, counters);
> > +
> > + node = uncore->nodes[node_id];
> > + if (!node) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit = kzalloc(sizeof(*unit), GFP_KERNEL);
> > + if (!unit) {
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > +
> > + unit->pdev = pdev;
> > + unit->map = ioremap(pci_resource_start(pdev, 0),
> > + pci_resource_len(pdev, 0));
> > + list_add(&unit->entry, &node->unit_list);
> > + node->nr_units++;
> > + found++;
> > + }
> > +
> > + if (!found)
> > + return -ENODEV;
> > +
> > + cpuhp_state_add_instance_nocalls(CPUHP_AP_UNCORE_CAVIUM_ONLINE,
> > + &uncore->node);
> > +
> > + /*
> > + * perf PMU is CPU dependent in difference to our uncore devices.
> > + * Just pick a CPU and migrate away if it goes offline.
> > + */
> > + cpumask_set_cpu(smp_processor_id(), &uncore->active_mask);
> > +
> > + uncore->pmu = *pmu;
> > + ret = perf_pmu_register(&uncore->pmu, uncore->pmu.name, -1);
> > + if (ret)
> > + goto fail;
> > +
> > + return 0;
> > +
> > +fail:
> > + node_id = 0;
> > + while (uncore->nodes[node_id]) {
> > + node = uncore->nodes[node_id];
> > +
> > + list_for_each_entry_safe(unit, tmp, &node->unit_list, entry) {
>
> Why do you need the _safe variant?
OK, not needed
> Will
^ permalink raw reply
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