[PATCH 0/2] drm: aspeed: Support more chip families

[PATCH 0/2] drm: aspeed: Support more chip families

[Joel Stanley]

The out of tree vendor driver was recently updated to support the
ast2400 and ast2600. These patches begin to add that support to the
mainline driver.

With these two cleanups it should be easier to support different
families of BMC system on chip with this driver.

I will merge them through drm-misc once they have been reviewed.

Joel Stanley (2):
  drm/aspeed: Look up syscon by phandle
  drm/aspeed: Use dt matching for default register values

 drivers/gpu/drm/aspeed/aspeed_gfx.h      |  7 +--
 drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 ++--
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c  | 58 +++++++++++++++++++-----
 3 files changed, 56 insertions(+), 19 deletions(-)

-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/2] drm: aspeed: Support more chip families

[Joel Stanley]

The out of tree vendor driver was recently updated to support the
ast2400 and ast2600. These patches begin to add that support to the
mainline driver.

With these two cleanups it should be easier to support different
families of BMC system on chip with this driver.

I will merge them through drm-misc once they have been reviewed.

Joel Stanley (2):
  drm/aspeed: Look up syscon by phandle
  drm/aspeed: Use dt matching for default register values

 drivers/gpu/drm/aspeed/aspeed_gfx.h      |  7 +--
 drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 ++--
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c  | 58 +++++++++++++++++++-----
 3 files changed, 56 insertions(+), 19 deletions(-)

-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Joel Stanley]

This scales better to multiple families of SoC. The lookup by compatible
can be removed in a future change.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 457ec04950f7..8ada7e944147 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -103,6 +103,7 @@ static int aspeed_gfx_load(struct drm_device *drm)
 {
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct aspeed_gfx *priv = to_aspeed_gfx(drm);
+	struct device_node *np = pdev->dev.of_node;
 	struct resource *res;
 	int ret;
 
@@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
+	priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(priv->scu)) {
-		dev_err(&pdev->dev, "failed to find SCU regmap\n");
-		return PTR_ERR(priv->scu);
+		priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
+		if (IS_ERR(priv->scu)) {
+			dev_err(&pdev->dev, "failed to find SCU regmap\n");
+			return PTR_ERR(priv->scu);
+		}
 	}
 
 	ret = of_reserved_mem_device_init(drm->dev);
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Joel Stanley]

This scales better to multiple families of SoC. The lookup by compatible
can be removed in a future change.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 457ec04950f7..8ada7e944147 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -103,6 +103,7 @@ static int aspeed_gfx_load(struct drm_device *drm)
 {
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct aspeed_gfx *priv = to_aspeed_gfx(drm);
+	struct device_node *np = pdev->dev.of_node;
 	struct resource *res;
 	int ret;
 
@@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
+	priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(priv->scu)) {
-		dev_err(&pdev->dev, "failed to find SCU regmap\n");
-		return PTR_ERR(priv->scu);
+		priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
+		if (IS_ERR(priv->scu)) {
+			dev_err(&pdev->dev, "failed to find SCU regmap\n");
+			return PTR_ERR(priv->scu);
+		}
 	}
 
 	ret = of_reserved_mem_device_init(drm->dev);
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Joel Stanley]

There are minor differences in the values for the threshold value and
the scan line size between families of ASPEED SoC. Additionally the SCU
register for the output control differs between families.

This adds device tree matching to parameterise these values, allowing us
to add support for the AST2400 now, and in the future the AST2600.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/gpu/drm/aspeed/aspeed_gfx.h      |  7 ++--
 drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 +++--
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c  | 48 +++++++++++++++++++-----
 3 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h b/drivers/gpu/drm/aspeed/aspeed_gfx.h
index f1e7e56abc02..c81d214688f2 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
@@ -11,6 +11,10 @@ struct aspeed_gfx {
 	struct reset_control		*rst;
 	struct regmap			*scu;
 
+	u32				dac_reg;
+	u32				throd_val;
+	u32				scan_line_max;
+
 	struct drm_simple_display_pipe	pipe;
 	struct drm_connector		connector;
 };
@@ -100,6 +104,3 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CRT_THROD */
 #define CRT_THROD_LOW(x)		(x)
 #define CRT_THROD_HIGH(x)		((x) << 8)
-
-/* Default Threshold Seting */
-#define G5_CRT_THROD_VAL	(CRT_THROD_LOW(0x24) | CRT_THROD_HIGH(0x3C))
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
index e54686c31a90..4e8126edacbd 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
@@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct aspeed_gfx *priv)
 	u32 ctrl2 = readl(priv->base + CRT_CTRL2);
 
 	/* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
-	regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
+	regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));
+
 
 	writel(ctrl1 | CRT_CTRL_EN, priv->base + CRT_CTRL1);
 	writel(ctrl2 | CRT_CTRL_DAC_EN, priv->base + CRT_CTRL2);
@@ -74,7 +75,7 @@ static void aspeed_gfx_disable_controller(struct aspeed_gfx *priv)
 	writel(ctrl1 & ~CRT_CTRL_EN, priv->base + CRT_CTRL1);
 	writel(ctrl2 & ~CRT_CTRL_DAC_EN, priv->base + CRT_CTRL2);
 
-	regmap_update_bits(priv->scu, 0x2c, BIT(16), 0);
+	regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), 0);
 }
 
 static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
@@ -127,7 +128,8 @@ static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
 	 * Terminal Count: memory size of one scan line
 	 */
 	d_offset = m->hdisplay * bpp / 8;
-	t_count = (m->hdisplay * bpp + 127) / 128;
+	t_count = DIV_ROUND_UP(m->hdisplay * bpp, priv->scan_line_max);
+
 	writel(CRT_DISP_OFFSET(d_offset) | CRT_TERM_COUNT(t_count),
 			priv->base + CRT_OFFSET);
 
@@ -135,7 +137,7 @@ static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
 	 * Threshold: FIFO thresholds of refill and stop (16 byte chunks
 	 * per line, rounded up)
 	 */
-	writel(G5_CRT_THROD_VAL, priv->base + CRT_THROD);
+	writel(priv->throd_val, priv->base + CRT_THROD);
 }
 
 static void aspeed_gfx_pipe_enable(struct drm_simple_display_pipe *pipe,
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 8ada7e944147..de0f0bf82c6b 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -7,6 +7,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -57,6 +58,31 @@
  * which is available under NDA from ASPEED.
  */
 
+struct aspeed_gfx_config {
+	u32 dac_reg;		/* DAC register in SCU */
+	u32 throd_val;		/* Default Threshold Seting */
+	u32 scan_line_max;	/* Max memory size of one scan line */
+};
+
+static const struct aspeed_gfx_config ast2400_config = {
+	.dac_reg = 0x2c,
+	.throd_val = CRT_THROD_LOW(0x1e) | CRT_THROD_HIGH(0x12),
+	.scan_line_max = 64,
+};
+
+static const struct aspeed_gfx_config ast2500_config = {
+	.dac_reg = 0x2c,
+	.throd_val = CRT_THROD_LOW(0x24) | CRT_THROD_HIGH(0x3c),
+	.scan_line_max = 128,
+};
+
+static const struct of_device_id aspeed_gfx_match[] = {
+	{ .compatible = "aspeed,ast2400-gfx", .data = &ast2400_config },
+	{ .compatible = "aspeed,ast2500-gfx", .data = &ast2500_config },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, aspeed_gfx_of_table);
+
 static const struct drm_mode_config_funcs aspeed_gfx_mode_config_funcs = {
 	.fb_create		= drm_gem_fb_create,
 	.atomic_check		= drm_atomic_helper_check,
@@ -97,13 +123,13 @@ static irqreturn_t aspeed_gfx_irq_handler(int irq, void *data)
 	return IRQ_NONE;
 }
 
-
-
 static int aspeed_gfx_load(struct drm_device *drm)
 {
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct aspeed_gfx *priv = to_aspeed_gfx(drm);
 	struct device_node *np = pdev->dev.of_node;
+	const struct aspeed_gfx_config *config;
+	const struct of_device_id *match;
 	struct resource *res;
 	int ret;
 
@@ -112,6 +138,15 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
+	match = of_match_device(aspeed_gfx_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+	config = match->data;
+
+	priv->dac_reg = config->dac_reg;
+	priv->throd_val = config->throd_val;
+	priv->scan_line_max = config->scan_line_max;
+
 	priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(priv->scu)) {
 		priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
@@ -206,11 +241,6 @@ static const struct drm_driver aspeed_gfx_driver = {
 	.minor = 0,
 };
 
-static const struct of_device_id aspeed_gfx_match[] = {
-	{ .compatible = "aspeed,ast2500-gfx" },
-	{ }
-};
-
 #define ASPEED_SCU_VGA0		0x50
 #define ASPEED_SCU_MISC_CTRL	0x2c
 
@@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev, struct device_attribute *attr,
 	if (val > 3)
 		return -EINVAL;
 
-	rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
+	rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
 	if (rc < 0)
 		return 0;
 
@@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev, struct device_attribute *attr, c
 	u32 reg;
 	int rc;
 
-	rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
+	rc = regmap_read(priv->scu, priv->dac_reg, &reg);
 	if (rc)
 		return rc;
 
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Joel Stanley]

There are minor differences in the values for the threshold value and
the scan line size between families of ASPEED SoC. Additionally the SCU
register for the output control differs between families.

This adds device tree matching to parameterise these values, allowing us
to add support for the AST2400 now, and in the future the AST2600.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/gpu/drm/aspeed/aspeed_gfx.h      |  7 ++--
 drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 +++--
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c  | 48 +++++++++++++++++++-----
 3 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx.h b/drivers/gpu/drm/aspeed/aspeed_gfx.h
index f1e7e56abc02..c81d214688f2 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx.h
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx.h
@@ -11,6 +11,10 @@ struct aspeed_gfx {
 	struct reset_control		*rst;
 	struct regmap			*scu;
 
+	u32				dac_reg;
+	u32				throd_val;
+	u32				scan_line_max;
+
 	struct drm_simple_display_pipe	pipe;
 	struct drm_connector		connector;
 };
@@ -100,6 +104,3 @@ int aspeed_gfx_create_output(struct drm_device *drm);
 /* CRT_THROD */
 #define CRT_THROD_LOW(x)		(x)
 #define CRT_THROD_HIGH(x)		((x) << 8)
-
-/* Default Threshold Seting */
-#define G5_CRT_THROD_VAL	(CRT_THROD_LOW(0x24) | CRT_THROD_HIGH(0x3C))
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
index e54686c31a90..4e8126edacbd 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
@@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct aspeed_gfx *priv)
 	u32 ctrl2 = readl(priv->base + CRT_CTRL2);
 
 	/* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
-	regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
+	regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));
+
 
 	writel(ctrl1 | CRT_CTRL_EN, priv->base + CRT_CTRL1);
 	writel(ctrl2 | CRT_CTRL_DAC_EN, priv->base + CRT_CTRL2);
@@ -74,7 +75,7 @@ static void aspeed_gfx_disable_controller(struct aspeed_gfx *priv)
 	writel(ctrl1 & ~CRT_CTRL_EN, priv->base + CRT_CTRL1);
 	writel(ctrl2 & ~CRT_CTRL_DAC_EN, priv->base + CRT_CTRL2);
 
-	regmap_update_bits(priv->scu, 0x2c, BIT(16), 0);
+	regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), 0);
 }
 
 static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
@@ -127,7 +128,8 @@ static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
 	 * Terminal Count: memory size of one scan line
 	 */
 	d_offset = m->hdisplay * bpp / 8;
-	t_count = (m->hdisplay * bpp + 127) / 128;
+	t_count = DIV_ROUND_UP(m->hdisplay * bpp, priv->scan_line_max);
+
 	writel(CRT_DISP_OFFSET(d_offset) | CRT_TERM_COUNT(t_count),
 			priv->base + CRT_OFFSET);
 
@@ -135,7 +137,7 @@ static void aspeed_gfx_crtc_mode_set_nofb(struct aspeed_gfx *priv)
 	 * Threshold: FIFO thresholds of refill and stop (16 byte chunks
 	 * per line, rounded up)
 	 */
-	writel(G5_CRT_THROD_VAL, priv->base + CRT_THROD);
+	writel(priv->throd_val, priv->base + CRT_THROD);
 }
 
 static void aspeed_gfx_pipe_enable(struct drm_simple_display_pipe *pipe,
diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index 8ada7e944147..de0f0bf82c6b 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -7,6 +7,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -57,6 +58,31 @@
  * which is available under NDA from ASPEED.
  */
 
+struct aspeed_gfx_config {
+	u32 dac_reg;		/* DAC register in SCU */
+	u32 throd_val;		/* Default Threshold Seting */
+	u32 scan_line_max;	/* Max memory size of one scan line */
+};
+
+static const struct aspeed_gfx_config ast2400_config = {
+	.dac_reg = 0x2c,
+	.throd_val = CRT_THROD_LOW(0x1e) | CRT_THROD_HIGH(0x12),
+	.scan_line_max = 64,
+};
+
+static const struct aspeed_gfx_config ast2500_config = {
+	.dac_reg = 0x2c,
+	.throd_val = CRT_THROD_LOW(0x24) | CRT_THROD_HIGH(0x3c),
+	.scan_line_max = 128,
+};
+
+static const struct of_device_id aspeed_gfx_match[] = {
+	{ .compatible = "aspeed,ast2400-gfx", .data = &ast2400_config },
+	{ .compatible = "aspeed,ast2500-gfx", .data = &ast2500_config },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, aspeed_gfx_of_table);
+
 static const struct drm_mode_config_funcs aspeed_gfx_mode_config_funcs = {
 	.fb_create		= drm_gem_fb_create,
 	.atomic_check		= drm_atomic_helper_check,
@@ -97,13 +123,13 @@ static irqreturn_t aspeed_gfx_irq_handler(int irq, void *data)
 	return IRQ_NONE;
 }
 
-
-
 static int aspeed_gfx_load(struct drm_device *drm)
 {
 	struct platform_device *pdev = to_platform_device(drm->dev);
 	struct aspeed_gfx *priv = to_aspeed_gfx(drm);
 	struct device_node *np = pdev->dev.of_node;
+	const struct aspeed_gfx_config *config;
+	const struct of_device_id *match;
 	struct resource *res;
 	int ret;
 
@@ -112,6 +138,15 @@ static int aspeed_gfx_load(struct drm_device *drm)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
+	match = of_match_device(aspeed_gfx_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+	config = match->data;
+
+	priv->dac_reg = config->dac_reg;
+	priv->throd_val = config->throd_val;
+	priv->scan_line_max = config->scan_line_max;
+
 	priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
 	if (IS_ERR(priv->scu)) {
 		priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
@@ -206,11 +241,6 @@ static const struct drm_driver aspeed_gfx_driver = {
 	.minor = 0,
 };
 
-static const struct of_device_id aspeed_gfx_match[] = {
-	{ .compatible = "aspeed,ast2500-gfx" },
-	{ }
-};
-
 #define ASPEED_SCU_VGA0		0x50
 #define ASPEED_SCU_MISC_CTRL	0x2c
 
@@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev, struct device_attribute *attr,
 	if (val > 3)
 		return -EINVAL;
 
-	rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
+	rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
 	if (rc < 0)
 		return 0;
 
@@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev, struct device_attribute *attr, c
 	u32 reg;
 	int rc;
 
-	rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
+	rc = regmap_read(priv->scu, priv->dac_reg, &reg);
 	if (rc)
 		return rc;
 
-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Jeremy Kerr]

Hi Joel,

Sounds like a good idea! One comment though:

> @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
>         if (IS_ERR(priv->base))
>                 return PTR_ERR(priv->base);
>  
> -       priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> +       priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
>         if (IS_ERR(priv->scu)) {
> -               dev_err(&pdev->dev, "failed to find SCU regmap\n");
> -               return PTR_ERR(priv->scu);
> +               priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");

Is this (more generic) compatible value guaranteed to exist alongside
aspeed,ast2500-scu? The scu binding only specifies the model-specific
ones:

    Documentation/devicetree/bindings/mfd/aspeed-scu.txt:

    Required properties:
    - compatible:	One of:
                    "aspeed,ast2400-scu", "syscon", "simple-mfd"
                    "aspeed,ast2500-scu", "syscon", "simple-mfd"

- the only mention of the new compatible value that I can find is this
thread. Maybe we should retain the existing one to keep the fallback
case working?

Cheers,


Jeremy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Jeremy Kerr]

Hi Joel,

Sounds like a good idea! One comment though:

> @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
>         if (IS_ERR(priv->base))
>                 return PTR_ERR(priv->base);
>  
> -       priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> +       priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
>         if (IS_ERR(priv->scu)) {
> -               dev_err(&pdev->dev, "failed to find SCU regmap\n");
> -               return PTR_ERR(priv->scu);
> +               priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");

Is this (more generic) compatible value guaranteed to exist alongside
aspeed,ast2500-scu? The scu binding only specifies the model-specific
ones:

    Documentation/devicetree/bindings/mfd/aspeed-scu.txt:

    Required properties:
    - compatible:	One of:
                    "aspeed,ast2400-scu", "syscon", "simple-mfd"
                    "aspeed,ast2500-scu", "syscon", "simple-mfd"

- the only mention of the new compatible value that I can find is this
thread. Maybe we should retain the existing one to keep the fallback
case working?

Cheers,


Jeremy

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Jeremy Kerr]

Hi Joel,

> There are minor differences in the values for the threshold value and
> the scan line size between families of ASPEED SoC. Additionally the
> SCU register for the output control differs between families.
> 
> This adds device tree matching to parameterise these values, allowing
> us to add support for the AST2400 now, and in the future the AST2600.

Looks good to me. Two super minor things:

> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> @@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct
> aspeed_gfx *priv)
>         u32 ctrl2 = readl(priv->base + CRT_CTRL2);
>  
>         /* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
> -       regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
> +       regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));

The comment references SCU2C; but you've implied that this will
change...

> @@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev,
> struct device_attribute *attr,
>         if (val > 3)
>                 return -EINVAL;
>  
> -       rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
> +       rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
>         if (rc < 0)
>                 return 0;
>  
> @@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev,
> struct device_attribute *attr, c
>         u32 reg;
>         int rc;
>  
> -       rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
> +       rc = regmap_read(priv->scu, priv->dac_reg, &reg);
>         if (rc)
>                 return rc;

You've removed the only uses of ASPEED_SCU_MISC_CTRL here, maybe drop
the #define too?

Regardless:

Reviewed-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Jeremy Kerr]

Hi Joel,

> There are minor differences in the values for the threshold value and
> the scan line size between families of ASPEED SoC. Additionally the
> SCU register for the output control differs between families.
> 
> This adds device tree matching to parameterise these values, allowing
> us to add support for the AST2400 now, and in the future the AST2600.

Looks good to me. Two super minor things:

> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> @@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct
> aspeed_gfx *priv)
>         u32 ctrl2 = readl(priv->base + CRT_CTRL2);
>  
>         /* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
> -       regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
> +       regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));

The comment references SCU2C; but you've implied that this will
change...

> @@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev,
> struct device_attribute *attr,
>         if (val > 3)
>                 return -EINVAL;
>  
> -       rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
> +       rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
>         if (rc < 0)
>                 return 0;
>  
> @@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev,
> struct device_attribute *attr, c
>         u32 reg;
>         int rc;
>  
> -       rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
> +       rc = regmap_read(priv->scu, priv->dac_reg, &reg);
>         if (rc)
>                 return rc;

You've removed the only uses of ASPEED_SCU_MISC_CTRL here, maybe drop
the #define too?

Regardless:

Reviewed-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Joel Stanley]

On Tue, 2 Feb 2021 at 04:39, Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Hi Joel,
>
> Sounds like a good idea! One comment though:
>
> > @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
> >         if (IS_ERR(priv->base))
> >                 return PTR_ERR(priv->base);
> >
> > -       priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> > +       priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
> >         if (IS_ERR(priv->scu)) {
> > -               dev_err(&pdev->dev, "failed to find SCU regmap\n");
> > -               return PTR_ERR(priv->scu);
> > +               priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
>
> Is this (more generic) compatible value guaranteed to exist alongside
> aspeed,ast2500-scu? The scu binding only specifies the model-specific
> ones:
>
>     Documentation/devicetree/bindings/mfd/aspeed-scu.txt:
>
>     Required properties:
>     - compatible:       One of:
>                     "aspeed,ast2400-scu", "syscon", "simple-mfd"
>                     "aspeed,ast2500-scu", "syscon", "simple-mfd"
>
> - the only mention of the new compatible value that I can find is this
> thread. Maybe we should retain the existing one to keep the fallback
> case working?

Yes, it was a mistake to change ast2500-scu to aspeed-scu. The only
reason to keep the lookup_by_compatible was to decouple this patch
from the device tree changes.

I will send a v2 with syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu").

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm/aspeed: Look up syscon by phandle

[Joel Stanley]

On Tue, 2 Feb 2021 at 04:39, Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Hi Joel,
>
> Sounds like a good idea! One comment though:
>
> > @@ -111,10 +112,13 @@ static int aspeed_gfx_load(struct drm_device *drm)
> >         if (IS_ERR(priv->base))
> >                 return PTR_ERR(priv->base);
> >
> > -       priv->scu = syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu");
> > +       priv->scu = syscon_regmap_lookup_by_phandle(np, "syscon");
> >         if (IS_ERR(priv->scu)) {
> > -               dev_err(&pdev->dev, "failed to find SCU regmap\n");
> > -               return PTR_ERR(priv->scu);
> > +               priv->scu = syscon_regmap_lookup_by_compatible("aspeed,aspeed-scu");
>
> Is this (more generic) compatible value guaranteed to exist alongside
> aspeed,ast2500-scu? The scu binding only specifies the model-specific
> ones:
>
>     Documentation/devicetree/bindings/mfd/aspeed-scu.txt:
>
>     Required properties:
>     - compatible:       One of:
>                     "aspeed,ast2400-scu", "syscon", "simple-mfd"
>                     "aspeed,ast2500-scu", "syscon", "simple-mfd"
>
> - the only mention of the new compatible value that I can find is this
> thread. Maybe we should retain the existing one to keep the fallback
> case working?

Yes, it was a mistake to change ast2500-scu to aspeed-scu. The only
reason to keep the lookup_by_compatible was to decouple this patch
from the device tree changes.

I will send a v2 with syscon_regmap_lookup_by_compatible("aspeed,ast2500-scu").
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Joel Stanley]

On Tue, 2 Feb 2021 at 04:46, Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Hi Joel,
>
> > There are minor differences in the values for the threshold value and
> > the scan line size between families of ASPEED SoC. Additionally the
> > SCU register for the output control differs between families.
> >
> > This adds device tree matching to parameterise these values, allowing
> > us to add support for the AST2400 now, and in the future the AST2600.
>
> Looks good to me. Two super minor things:
>
> > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > @@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct
> > aspeed_gfx *priv)
> >         u32 ctrl2 = readl(priv->base + CRT_CTRL2);
> >
> >         /* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
> > -       regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
> > +       regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));
>
> The comment references SCU2C; but you've implied that this will
> change...
>
> > @@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev,
> > struct device_attribute *attr,
> >         if (val > 3)
> >                 return -EINVAL;
> >
> > -       rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
> > +       rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
> >         if (rc < 0)
> >                 return 0;
> >
> > @@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev,
> > struct device_attribute *attr, c
> >         u32 reg;
> >         int rc;
> >
> > -       rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
> > +       rc = regmap_read(priv->scu, priv->dac_reg, &reg);
> >         if (rc)
> >                 return rc;
>
> You've removed the only uses of ASPEED_SCU_MISC_CTRL here, maybe drop
> the #define too?

Good idea. I'll implement both of your suggestions.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] drm/aspeed: Use dt matching for default register values

[Joel Stanley]

On Tue, 2 Feb 2021 at 04:46, Jeremy Kerr <jk@ozlabs.org> wrote:
>
> Hi Joel,
>
> > There are minor differences in the values for the threshold value and
> > the scan line size between families of ASPEED SoC. Additionally the
> > SCU register for the output control differs between families.
> >
> > This adds device tree matching to parameterise these values, allowing
> > us to add support for the AST2400 now, and in the future the AST2600.
>
> Looks good to me. Two super minor things:
>
> > --- a/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c
> > @@ -60,7 +60,8 @@ static void aspeed_gfx_enable_controller(struct
> > aspeed_gfx *priv)
> >         u32 ctrl2 = readl(priv->base + CRT_CTRL2);
> >
> >         /* SCU2C: set DAC source for display output to Graphics CRT (GFX) */
> > -       regmap_update_bits(priv->scu, 0x2c, BIT(16), BIT(16));
> > +       regmap_update_bits(priv->scu, priv->dac_reg, BIT(16), BIT(16));
>
> The comment references SCU2C; but you've implied that this will
> change...
>
> > @@ -228,7 +258,7 @@ static ssize_t dac_mux_store(struct device *dev,
> > struct device_attribute *attr,
> >         if (val > 3)
> >                 return -EINVAL;
> >
> > -       rc = regmap_update_bits(priv->scu, ASPEED_SCU_MISC_CTRL, 0x30000, val << 16);
> > +       rc = regmap_update_bits(priv->scu, priv->dac_reg, 0x30000, val << 16);
> >         if (rc < 0)
> >                 return 0;
> >
> > @@ -241,7 +271,7 @@ static ssize_t dac_mux_show(struct device *dev,
> > struct device_attribute *attr, c
> >         u32 reg;
> >         int rc;
> >
> > -       rc = regmap_read(priv->scu, ASPEED_SCU_MISC_CTRL, &reg);
> > +       rc = regmap_read(priv->scu, priv->dac_reg, &reg);
> >         if (rc)
> >                 return rc;
>
> You've removed the only uses of ASPEED_SCU_MISC_CTRL here, maybe drop
> the #define too?

Good idea. I'll implement both of your suggestions.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel