[PATCH 0/7] add LCD support for SAM9X60

[PATCH 0/7] add LCD support for SAM9X60

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Hi,

These patches adds support for SAM9X60's LCD controller.

First patches add option to specify if controller clock source is fixed.
Second patch avoid a variable initialization in atmel_hlcdc_crtc_mode_set_nofb().
The 3rd one adds specific bindings for SAM9X60 LCD controller.
The 4th and 5th add compatibles in the driver.
The 6th patch enables sys_clk in probe since SAM9X60 needs this.
Specific support was added also in suspend/resume hooks.
The work in the 6th patch was done based on support added in 1st patch.
The 7th patch adds SAM9X60's LCD configuration and enabled it. 

I kept a big series including PWM, MFD, LCD changes due to shared DT
bindings. If you prefer, I'm available to send them separately. Please let
me know.

Thank you,
Claudiu Beznea

Claudiu Beznea (5):
  drm: atmel-hlcdc: add config option for clock selection
  drm: atmel-hlcdc: avoid initializing cfg with zero
  dt-bindings: mfd: add bindings for SAM9X60 HLCD controller
  mfd: atmel-hlcdc: add compatible for SAM9X60 HLCD controller
  pwm: atmel-hlcdc: add compatible for SAM9X60 HLCDC's PWM

Sandeep Sheriker Mallikarjun (2):
  drm: atmel-hlcdc: enable sys_clk during initalization.
  drm: atmel-hlcdc: add sam9x60 LCD controller

 .../devicetree/bindings/mfd/atmel-hlcdc.txt        |   1 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c     |  17 ++-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c       | 120 ++++++++++++++++++++-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h       |   2 +
 drivers/mfd/atmel-hlcdc.c                          |   1 +
 drivers/pwm/pwm-atmel-hlcdc.c                      |   3 +
 6 files changed, 134 insertions(+), 10 deletions(-)

-- 
2.7.4

[PATCH 1/7] drm: atmel-hlcdc: add config option for clock selection

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

SAM9x60 LCD Controller has no option to select clock source as previous
controllers have. To be able to use the same driver even for this LCD
controller add a config option to know if controller supports this.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 +++++++-----
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  2 ++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 8070a558d7b1..17a7a18f6a07 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -78,7 +78,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 	unsigned long mode_rate;
 	struct videomode vm;
 	unsigned long prate;
-	unsigned int cfg;
+	unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL;
+	unsigned int cfg = 0;
 	int div;
 
 	vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay;
@@ -101,7 +102,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 		     (adj->crtc_hdisplay - 1) |
 		     ((adj->crtc_vdisplay - 1) << 16));
 
-	cfg = ATMEL_HLCDC_CLKSEL;
+	if (!crtc->dc->desc->fixed_clksrc) {
+		cfg = ATMEL_HLCDC_CLKSEL;
+		mask |= ATMEL_HLCDC_CLKSEL;
+	}
 
 	prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk);
 	mode_rate = adj->crtc_clock * 1000;
@@ -132,9 +136,7 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 
 	cfg |= ATMEL_HLCDC_CLKDIV(div);
 
-	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0),
-			   ATMEL_HLCDC_CLKSEL | ATMEL_HLCDC_CLKDIV_MASK |
-			   ATMEL_HLCDC_CLKPOL, cfg);
+	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg);
 
 	cfg = 0;
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index 70bd540d644e..0155efb9c443 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -328,6 +328,7 @@ atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *layer)
  * @max_hpw: maximum horizontal back/front porch width
  * @conflicting_output_formats: true if RGBXXX output formats conflict with
  *				each other.
+ * @fixed_clksrc: true if clock source is fixed
  * @layers: a layer description table describing available layers
  * @nlayers: layer description table size
  */
@@ -340,6 +341,7 @@ struct atmel_hlcdc_dc_desc {
 	int max_vpw;
 	int max_hpw;
 	bool conflicting_output_formats;
+	bool fixed_clksrc;
 	const struct atmel_hlcdc_layer_desc *layers;
 	int nlayers;
 };
-- 
2.7.4

[PATCH 2/7] drm: atmel-hlcdc: avoid initializing cfg with zero

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Remove cfg initialization with zero and read state with
drm_crtc_state_to_atmel_hlcdc_crtc_state() so that cfg to be initialized
with state's output_mode.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
index 17a7a18f6a07..7b0c6683a690 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
@@ -138,7 +138,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 
 	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg);
 
-	cfg = 0;
+	state = drm_crtc_state_to_atmel_hlcdc_crtc_state(c->state);
+	cfg = state->output_mode << 8;
 
 	if (adj->flags & DRM_MODE_FLAG_NVSYNC)
 		cfg |= ATMEL_HLCDC_VSPOL;
@@ -146,9 +147,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
 	if (adj->flags & DRM_MODE_FLAG_NHSYNC)
 		cfg |= ATMEL_HLCDC_HSPOL;
 
-	state = drm_crtc_state_to_atmel_hlcdc_crtc_state(c->state);
-	cfg |= state->output_mode << 8;
-
 	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(5),
 			   ATMEL_HLCDC_HSPOL | ATMEL_HLCDC_VSPOL |
 			   ATMEL_HLCDC_VSPDLYS | ATMEL_HLCDC_VSPDLYE |
-- 
2.7.4

[PATCH 3/7] dt-bindings: mfd: add bindings for SAM9X60 HLCD controller

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add new compatible string for the HLCD controller on SAM9X60 SoC.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt b/Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt
index 3f643ef121ff..5f8880cc757e 100644
--- a/Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt
+++ b/Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt
@@ -7,6 +7,7 @@ Required properties:
    "atmel,sama5d2-hlcdc"
    "atmel,sama5d3-hlcdc"
    "atmel,sama5d4-hlcdc"
+   "microchip,sam9x60-hlcdc"
  - reg: base address and size of the HLCDC device registers.
  - clock-names: the name of the 3 clocks requested by the HLCDC device.
    Should contain "periph_clk", "sys_clk" and "slow_clk".
-- 
2.7.4

[PATCH 4/7] mfd: atmel-hlcdc: add compatible for SAM9X60 HLCD controller

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add compatible for SAM9X60 HLCD controller.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/mfd/atmel-hlcdc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c
index e82543bcfdc8..35a9e16f9902 100644
--- a/drivers/mfd/atmel-hlcdc.c
+++ b/drivers/mfd/atmel-hlcdc.c
@@ -141,6 +141,7 @@ static const struct of_device_id atmel_hlcdc_match[] = {
 	{ .compatible = "atmel,sama5d2-hlcdc" },
 	{ .compatible = "atmel,sama5d3-hlcdc" },
 	{ .compatible = "atmel,sama5d4-hlcdc" },
+	{ .compatible = "microchip,sam9x60-hlcdc" },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, atmel_hlcdc_match);
-- 
2.7.4

[PATCH 5/7] pwm: atmel-hlcdc: add compatible for SAM9X60 HLCDC's PWM

[Claudiu.Beznea]

From: Claudiu Beznea <claudiu.beznea@microchip.com>

Add compatible string for SAM9X60 HLCDC's PWM.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-atmel-hlcdc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/pwm/pwm-atmel-hlcdc.c b/drivers/pwm/pwm-atmel-hlcdc.c
index 54c6633d9b5d..331ca0233d9e 100644
--- a/drivers/pwm/pwm-atmel-hlcdc.c
+++ b/drivers/pwm/pwm-atmel-hlcdc.c
@@ -246,6 +246,9 @@ static const struct of_device_id atmel_hlcdc_dt_ids[] = {
 		.compatible = "atmel,sama5d4-hlcdc",
 		.data = &atmel_hlcdc_pwm_sama5d3_errata,
 	},
+	{
+		.compatible = "microchip,sam9x60-hlcdc",
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, atmel_hlcdc_dt_ids);
-- 
2.7.4

[PATCH 6/7] drm: atmel-hlcdc: enable sys_clk during initalization.

[Claudiu.Beznea]

From: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>

For SAM9X60 SoC, sys_clk is through lcd_gclk clock source and this
needs to be enabled before enabling lcd_clk.

Signed-off-by: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
[claudiu.beznea@microchip.com: add fixed_clksrc checks]
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 0be13eceedba..8bf51f853721 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -625,10 +625,18 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
 	dev->dev_private = dc;
 
+	if (dc->desc->fixed_clksrc) {
+		ret = clk_prepare_enable(dc->hlcdc->sys_clk);
+		if (ret) {
+			dev_err(dev->dev, "failed to enable sys_clk\n");
+			goto err_destroy_wq;
+		}
+	}
+
 	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
 	if (ret) {
 		dev_err(dev->dev, "failed to enable periph_clk\n");
-		goto err_destroy_wq;
+		goto err_sys_clk_disable;
 	}
 
 	pm_runtime_enable(dev->dev);
@@ -664,6 +672,9 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 err_periph_clk_disable:
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
+err_sys_clk_disable:
+	if (dc->desc->fixed_clksrc)
+		clk_disable_unprepare(dc->hlcdc->sys_clk);
 
 err_destroy_wq:
 	destroy_workqueue(dc->wq);
@@ -688,6 +699,8 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
+	if (dc->desc->fixed_clksrc)
+		clk_disable_unprepare(dc->hlcdc->sys_clk);
 	destroy_workqueue(dc->wq);
 }
 
@@ -805,6 +818,8 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
 	regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
 	regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
+	if (dc->desc->fixed_clksrc)
+		clk_disable_unprepare(dc->hlcdc->sys_clk);
 
 	return 0;
 }
@@ -814,6 +829,8 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev)
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
 	struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
 
+	if (dc->desc->fixed_clksrc)
+		clk_prepare_enable(dc->hlcdc->sys_clk);
 	clk_prepare_enable(dc->hlcdc->periph_clk);
 	regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
 
-- 
2.7.4

[PATCH 7/7] drm: atmel-hlcdc: add sam9x60 LCD controller

[Claudiu.Beznea]

From: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>

Add the LCD controller for SAM9X60.

Signed-off-by: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
[claudiu.beznea@microchip.com: add fixed_clksrc option to
 atmel_hlcdc_dc_sam9x60]
Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 101 +++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 8bf51f853721..fb2e7646daeb 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -364,6 +364,103 @@ static const struct atmel_hlcdc_dc_desc atmel_hlcdc_dc_sama5d4 = {
 	.nlayers = ARRAY_SIZE(atmel_hlcdc_sama5d4_layers),
 	.layers = atmel_hlcdc_sama5d4_layers,
 };
+
+static const struct atmel_hlcdc_layer_desc atmel_hlcdc_sam9x60_layers[] = {
+	{
+		.name = "base",
+		.formats = &atmel_hlcdc_plane_rgb_formats,
+		.regs_offset = 0x60,
+		.id = 0,
+		.type = ATMEL_HLCDC_BASE_LAYER,
+		.cfgs_offset = 0x2c,
+		.layout = {
+			.xstride = { 2 },
+			.default_color = 3,
+			.general_config = 4,
+			.disc_pos = 5,
+			.disc_size = 6,
+		},
+		.clut_offset = 0x600,
+	},
+	{
+		.name = "overlay1",
+		.formats = &atmel_hlcdc_plane_rgb_formats,
+		.regs_offset = 0x160,
+		.id = 1,
+		.type = ATMEL_HLCDC_OVERLAY_LAYER,
+		.cfgs_offset = 0x2c,
+		.layout = {
+			.pos = 2,
+			.size = 3,
+			.xstride = { 4 },
+			.pstride = { 5 },
+			.default_color = 6,
+			.chroma_key = 7,
+			.chroma_key_mask = 8,
+			.general_config = 9,
+		},
+		.clut_offset = 0xa00,
+	},
+	{
+		.name = "overlay2",
+		.formats = &atmel_hlcdc_plane_rgb_formats,
+		.regs_offset = 0x260,
+		.id = 2,
+		.type = ATMEL_HLCDC_OVERLAY_LAYER,
+		.cfgs_offset = 0x2c,
+		.layout = {
+			.pos = 2,
+			.size = 3,
+			.xstride = { 4 },
+			.pstride = { 5 },
+			.default_color = 6,
+			.chroma_key = 7,
+			.chroma_key_mask = 8,
+			.general_config = 9,
+		},
+		.clut_offset = 0xe00,
+	},
+	{
+		.name = "high-end-overlay",
+		.formats = &atmel_hlcdc_plane_rgb_and_yuv_formats,
+		.regs_offset = 0x360,
+		.id = 3,
+		.type = ATMEL_HLCDC_OVERLAY_LAYER,
+		.cfgs_offset = 0x4c,
+		.layout = {
+			.pos = 2,
+			.size = 3,
+			.memsize = 4,
+			.xstride = { 5, 7 },
+			.pstride = { 6, 8 },
+			.default_color = 9,
+			.chroma_key = 10,
+			.chroma_key_mask = 11,
+			.general_config = 12,
+			.scaler_config = 13,
+			.phicoeffs = {
+				.x = 17,
+				.y = 33,
+			},
+			.csc = 14,
+		},
+		.clut_offset = 0x1200,
+	},
+};
+
+static const struct atmel_hlcdc_dc_desc atmel_hlcdc_dc_sam9x60 = {
+	.min_width = 0,
+	.min_height = 0,
+	.max_width = 2048,
+	.max_height = 2048,
+	.max_spw = 0xff,
+	.max_vpw = 0xff,
+	.max_hpw = 0x3ff,
+	.fixed_clksrc = true,
+	.nlayers = ARRAY_SIZE(atmel_hlcdc_sam9x60_layers),
+	.layers = atmel_hlcdc_sam9x60_layers,
+};
+
 static const struct of_device_id atmel_hlcdc_of_match[] = {
 	{
 		.compatible = "atmel,at91sam9n12-hlcdc",
@@ -385,6 +482,10 @@ static const struct of_device_id atmel_hlcdc_of_match[] = {
 		.compatible = "atmel,sama5d4-hlcdc",
 		.data = &atmel_hlcdc_dc_sama5d4,
 	},
+	{
+		.compatible = "microchip,sam9x60-hlcdc",
+		.data = &atmel_hlcdc_dc_sam9x60,
+	},
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, atmel_hlcdc_of_match);
-- 
2.7.4

Re: [PATCH 0/7] add LCD support for SAM9X60

[Sam Ravnborg]

Hi Claudiu

On Wed, Feb 27, 2019 at 04:24:11PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Hi,
> 
> These patches adds support for SAM9X60's LCD controller.
Can you elaborate a little more which chips that are relevant.
To be able to look into the right data-sheet, while reviewing.
Link to data-sheet would be nice too.

Thanks in advance,
	Sam

Re: [PATCH 0/7] add LCD support for SAM9X60

[Alexandre Belloni]

On 28/02/2019 19:21:19+0100, Sam Ravnborg wrote:
> Hi Claudiu
> 
> On Wed, Feb 27, 2019 at 04:24:11PM +0000, Claudiu.Beznea@microchip.com wrote:
> > From: Claudiu Beznea <claudiu.beznea@microchip.com>
> > 
> > Hi,
> > 
> > These patches adds support for SAM9X60's LCD controller.
> Can you elaborate a little more which chips that are relevant.
> To be able to look into the right data-sheet, while reviewing.
> Link to data-sheet would be nice too.
> 

sam9x60 is the chip name (i.e. x is not a wildcard). I don't think the
datasheet is finished/public yet.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/7] add LCD support for SAM9X60

[Sam Ravnborg]

Hi Alexandre.

> > > 
> > > These patches adds support for SAM9X60's LCD controller.
> > Can you elaborate a little more which chips that are relevant.
> > To be able to look into the right data-sheet, while reviewing.
> > Link to data-sheet would be nice too.
> > 
> 
> sam9x60 is the chip name (i.e. x is not a wildcard). I don't think the
> datasheet is finished/public yet.

Thanks. I will take a look at the patches but not knowing
the specifics of the implementation/IP will makes this less valueable.

	Sam

Re: [PATCH 1/7] drm: atmel-hlcdc: add config option for clock selection

[Sam Ravnborg]

Hi Claudiu

On Wed, Feb 27, 2019 at 04:24:16PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> SAM9x60 LCD Controller has no option to select clock source as previous
> controllers have. To be able to use the same driver even for this LCD
> controller add a config option to know if controller supports this.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 +++++++-----
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  2 ++
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 8070a558d7b1..17a7a18f6a07 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -78,7 +78,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  	unsigned long mode_rate;
>  	struct videomode vm;
>  	unsigned long prate;
> -	unsigned int cfg;
> +	unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL;
> +	unsigned int cfg = 0;
>  	int div;
Here cfg and mask has initial values.

>  
>  	vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay;
> @@ -101,7 +102,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  		     (adj->crtc_hdisplay - 1) |
>  		     ((adj->crtc_vdisplay - 1) << 16));
>  
> -	cfg = ATMEL_HLCDC_CLKSEL;
> +	if (!crtc->dc->desc->fixed_clksrc) {
> +		cfg = ATMEL_HLCDC_CLKSEL;
> +		mask |= ATMEL_HLCDC_CLKSEL;
> +	}
And here mask is bitwise or with ATMEL_HLCDC_CLKSEL - fine.
But cfg is directly assigned.
If cfg was bitwise or the ATMEL_HLCDC_CLKSEL value then the code was prepared
to have extra flags added above these lines.

Only a small change and no functional difference.

With or without this change:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH 1/7] drm: atmel-hlcdc: add config option for clock selection

[Sam Ravnborg]

Hi Claudiu

One more reply to this patch...

On Wed, Feb 27, 2019 at 04:24:16PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> SAM9x60 LCD Controller has no option to select clock source as previous
> controllers have. To be able to use the same driver even for this LCD
> controller add a config option to know if controller supports this.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 +++++++-----
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  2 ++
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 8070a558d7b1..17a7a18f6a07 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -78,7 +78,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  	unsigned long mode_rate;
>  	struct videomode vm;
>  	unsigned long prate;
> -	unsigned int cfg;
> +	unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL;
> +	unsigned int cfg = 0;
>  	int div;
>  
>  	vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay;
> @@ -101,7 +102,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  		     (adj->crtc_hdisplay - 1) |
>  		     ((adj->crtc_vdisplay - 1) << 16));
>  
> -	cfg = ATMEL_HLCDC_CLKSEL;
> +	if (!crtc->dc->desc->fixed_clksrc) {
> +		cfg = ATMEL_HLCDC_CLKSEL;
> +		mask |= ATMEL_HLCDC_CLKSEL;
> +	}
Maybe this is just too late to look at code, but I do not get this.
If the sam9x60 do not support selecting the clk rate then I assume it is either
fixed to used system clock or 2 x system clock.

And we have in the driver code to adjust the "div" value.
The value of div depends on the CLKSEL configuration, so there must
be one part of this that is not valid when fixed_clksrc is true.

>  
>  	prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk);
>  	mode_rate = adj->crtc_clock * 1000;
> @@ -132,9 +136,7 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>  
>  	cfg |= ATMEL_HLCDC_CLKDIV(div);

It is the following code (not visible in this patch I talk about:
       if (div < 2) {
                div = 2;
        } else if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) {
                /* The divider ended up too big, try a lower base rate. */
                cfg &= ~ATMEL_HLCDC_CLKSEL;
                prate /= 2;
                div = DIV_ROUND_UP(prate, mode_rate);
                if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
                        div = ATMEL_HLCDC_CLKDIV_MASK;
        } else {
                int div_low = prate / mode_rate;

                if (div_low >= 2 &&
                    ((prate / div_low - mode_rate) <
                     10 * (mode_rate - prate / div)))
                        /*
                         * At least 10 times better when using a higher
                         * frequency than requested, instead of a lower.
                         * So, go with that.
                         */
                        div = div_low;
        }

Am I missing something obvious?

> -	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0),
> -			   ATMEL_HLCDC_CLKSEL | ATMEL_HLCDC_CLKDIV_MASK |
> -			   ATMEL_HLCDC_CLKPOL, cfg);
> +	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg);
>  
>  	cfg = 0;
>  
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> index 70bd540d644e..0155efb9c443 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
> @@ -328,6 +328,7 @@ atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *layer)
>   * @max_hpw: maximum horizontal back/front porch width
>   * @conflicting_output_formats: true if RGBXXX output formats conflict with
>   *				each other.
> + * @fixed_clksrc: true if clock source is fixed
Be specific here. Tell if "fixed_clksrc == true" equals system clock or 2 x system clock.

	Sam

Re: [PATCH 2/7] drm: atmel-hlcdc: avoid initializing cfg with zero

[Sam Ravnborg]

On Wed, Feb 27, 2019 at 04:24:21PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Remove cfg initialization with zero and read state with
> drm_crtc_state_to_atmel_hlcdc_crtc_state() so that cfg to be initialized
> with state's output_mode.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Looks good, good to have simpler code.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH 6/7] drm: atmel-hlcdc: enable sys_clk during initalization.

[Sam Ravnborg]

Hi Claudiu

On Wed, Feb 27, 2019 at 04:24:40PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
> 
> For SAM9X60 SoC, sys_clk is through lcd_gclk clock source and this
> needs to be enabled before enabling lcd_clk.

We have "ownership" of the clocks in the mfd device.
So it would make more sense to let the mfd device handle
the base clocks.
In other words - what about pushing enable of perigh_clk and sys_clk to
the mfd driver.

This may have the nice side-effect that we avoid
that both the drm driver and the pwm driver enable/disable the periph_clk
as it is today.

Another comment - fixed_clksrc is used to determine if sys_clk is enabled.
But that flag is about the clksource selection, and it is just a coincidence
that the same flag can be used here.
Why we cannot always enable sys_clk? Do we need to do this only
for sam9x60?
IF yes, then add a new falg. If no, then skip the flag.

	Sam


> 
> Signed-off-by: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
> [claudiu.beznea@microchip.com: add fixed_clksrc checks]
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 0be13eceedba..8bf51f853721 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -625,10 +625,18 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
>  	dev->dev_private = dc;
>  
> +	if (dc->desc->fixed_clksrc) {
> +		ret = clk_prepare_enable(dc->hlcdc->sys_clk);
> +		if (ret) {
> +			dev_err(dev->dev, "failed to enable sys_clk\n");
> +			goto err_destroy_wq;
> +		}
> +	}
> +
>  	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
>  	if (ret) {
>  		dev_err(dev->dev, "failed to enable periph_clk\n");
> -		goto err_destroy_wq;
> +		goto err_sys_clk_disable;
>  	}
>  
>  	pm_runtime_enable(dev->dev);
> @@ -664,6 +672,9 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>  err_periph_clk_disable:
>  	pm_runtime_disable(dev->dev);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> +err_sys_clk_disable:
> +	if (dc->desc->fixed_clksrc)
> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  
>  err_destroy_wq:
>  	destroy_workqueue(dc->wq);
> @@ -688,6 +699,8 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>  
>  	pm_runtime_disable(dev->dev);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> +	if (dc->desc->fixed_clksrc)
> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  	destroy_workqueue(dc->wq);
>  }
>  
> @@ -805,6 +818,8 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>  	regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>  	regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
> +	if (dc->desc->fixed_clksrc)
> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>  
>  	return 0;
>  }
> @@ -814,6 +829,8 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>  	struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>  
> +	if (dc->desc->fixed_clksrc)
> +		clk_prepare_enable(dc->hlcdc->sys_clk);
>  	clk_prepare_enable(dc->hlcdc->periph_clk);
>  	regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/7] drm: atmel-hlcdc: add config option for clock selection

[Claudiu.Beznea]

On 28.02.2019 23:38, Sam Ravnborg wrote:
> Hi Claudiu
> 
> One more reply to this patch...
> 
> On Wed, Feb 27, 2019 at 04:24:16PM +0000, Claudiu.Beznea@microchip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> SAM9x60 LCD Controller has no option to select clock source as previous
>> controllers have. To be able to use the same driver even for this LCD
>> controller add a config option to know if controller supports this.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 +++++++-----
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  2 ++
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 8070a558d7b1..17a7a18f6a07 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -78,7 +78,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  	unsigned long mode_rate;
>>  	struct videomode vm;
>>  	unsigned long prate;
>> -	unsigned int cfg;
>> +	unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL;
>> +	unsigned int cfg = 0;
>>  	int div;
>>  
>>  	vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay;
>> @@ -101,7 +102,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  		     (adj->crtc_hdisplay - 1) |
>>  		     ((adj->crtc_vdisplay - 1) << 16));
>>  
>> -	cfg = ATMEL_HLCDC_CLKSEL;
>> +	if (!crtc->dc->desc->fixed_clksrc) {
>> +		cfg = ATMEL_HLCDC_CLKSEL;
>> +		mask |= ATMEL_HLCDC_CLKSEL;
>> +	}
> Maybe this is just too late to look at code, but I do not get this.
> If the sam9x60 do not support selecting the clk rate then I assume it is either
> fixed to used system clock or 2 x system clock.

SAM9X60's LCD controller has no support for selecting clock source (and for
the moment, yes, as Alexandre specified, the datasheet is not public). If
you look at e.g. SAM9X35 datasheet , chapter 45.7, at LCD Controller
Configuration Register 0 [1], you will see this CLKSEL bit. On SAM9X60 you
don't have this option, but you have only one clock source, fixed one. You
still have the option to divide this clock to get LCD pixel clock using
CLKDIV field of LCD Controller Configuration Register 0, and the formula is
the same as in SAM9X35 case: the only difference is that the formula uses
the fixed clock in case of SAM9X60 apart from SAM9X35 which uses the
selected clock:
SAM9X60: LCDPCLK = source clock / (CLKDIV+2)
SAM9X35: pixel_clock = selected_clock / (CLKDIV + 2).

[1]
http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-11055-32-bit-ARM926EJ-S-Microcontroller-SAM9X35_Datasheet.pdf#G38.1471048

> 
> And we have in the driver code to adjust the "div" value.
> The value of div depends on the CLKSEL configuration, so there must
> be one part of this that is not valid when fixed_clksrc is true.

When fixed_clksrc is true you still have to option to divide the source clock.

I hope I was clear.

> 
>>  
>>  	prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk);
>>  	mode_rate = adj->crtc_clock * 1000;
>> @@ -132,9 +136,7 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  
>>  	cfg |= ATMEL_HLCDC_CLKDIV(div);
> 
> It is the following code (not visible in this patch I talk about:
>        if (div < 2) {
>                 div = 2;
>         } else if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) {
>                 /* The divider ended up too big, try a lower base rate. */
>                 cfg &= ~ATMEL_HLCDC_CLKSEL;
>                 prate /= 2;
>                 div = DIV_ROUND_UP(prate, mode_rate);
>                 if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK)
>                         div = ATMEL_HLCDC_CLKDIV_MASK;
>         } else {
>                 int div_low = prate / mode_rate;
> 
>                 if (div_low >= 2 &&
>                     ((prate / div_low - mode_rate) <
>                      10 * (mode_rate - prate / div)))
>                         /*
>                          * At least 10 times better when using a higher
>                          * frequency than requested, instead of a lower.
>                          * So, go with that.
>                          */
>                         div = div_low;
>         }
> 
> Am I missing something obvious?
> 
>> -	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0),
>> -			   ATMEL_HLCDC_CLKSEL | ATMEL_HLCDC_CLKDIV_MASK |
>> -			   ATMEL_HLCDC_CLKPOL, cfg);
>> +	regmap_update_bits(regmap, ATMEL_HLCDC_CFG(0), mask, cfg);
>>  
>>  	cfg = 0;
>>  
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
>> index 70bd540d644e..0155efb9c443 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
>> @@ -328,6 +328,7 @@ atmel_hlcdc_layer_to_plane(struct atmel_hlcdc_layer *layer)
>>   * @max_hpw: maximum horizontal back/front porch width
>>   * @conflicting_output_formats: true if RGBXXX output formats conflict with
>>   *				each other.
>> + * @fixed_clksrc: true if clock source is fixed
> Be specific here. Tell if "fixed_clksrc == true" equals system clock or 2 x system clock.

In case of SAM9X60, as I specified above, there is no option to select b/w
MCK and 2xMCK as in case of, e.g. SAM9X35, but only one clock source.

> 
> 	Sam
> 

Re: [PATCH 1/7] drm: atmel-hlcdc: add config option for clock selection

[Claudiu.Beznea]

On 28.02.2019 23:25, Sam Ravnborg wrote:
> Hi Claudiu
> 
> On Wed, Feb 27, 2019 at 04:24:16PM +0000, Claudiu.Beznea@microchip.com wrote:
>> From: Claudiu Beznea <claudiu.beznea@microchip.com>
>>
>> SAM9x60 LCD Controller has no option to select clock source as previous
>> controllers have. To be able to use the same driver even for this LCD
>> controller add a config option to know if controller supports this.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 +++++++-----
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h   |  2 ++
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> index 8070a558d7b1..17a7a18f6a07 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
>> @@ -78,7 +78,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  	unsigned long mode_rate;
>>  	struct videomode vm;
>>  	unsigned long prate;
>> -	unsigned int cfg;
>> +	unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL;
>> +	unsigned int cfg = 0;
>>  	int div;
> Here cfg and mask has initial values.
> 
>>  
>>  	vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay;
>> @@ -101,7 +102,10 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c)
>>  		     (adj->crtc_hdisplay - 1) |
>>  		     ((adj->crtc_vdisplay - 1) << 16));
>>  
>> -	cfg = ATMEL_HLCDC_CLKSEL;
>> +	if (!crtc->dc->desc->fixed_clksrc) {
>> +		cfg = ATMEL_HLCDC_CLKSEL;
>> +		mask |= ATMEL_HLCDC_CLKSEL;
>> +	}
> And here mask is bitwise or with ATMEL_HLCDC_CLKSEL - fine.
> But cfg is directly assigned.

I directly assigned it here because it was the first time it was used.
I will use a "|" operation as you suggest in next version.

> If cfg was bitwise or the ATMEL_HLCDC_CLKSEL value then the code was prepared
> to have extra flags added above these lines.
> 
> Only a small change and no functional difference.
> 
> With or without this change:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> 

Re: [PATCH 6/7] drm: atmel-hlcdc: enable sys_clk during initalization.

[Claudiu.Beznea]

On 28.02.2019 23:55, Sam Ravnborg wrote:
> Hi Claudiu
> 
> On Wed, Feb 27, 2019 at 04:24:40PM +0000, Claudiu.Beznea@microchip.com wrote:
>> From: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
>>
>> For SAM9X60 SoC, sys_clk is through lcd_gclk clock source and this
>> needs to be enabled before enabling lcd_clk.
> 
> We have "ownership" of the clocks in the mfd device.
> So it would make more sense to let the mfd device handle
> the base clocks.
> In other words - what about pushing enable of perigh_clk and sys_clk to
> the mfd driver.

I think this could be achieved, but how is better? To have individual
drivers (in this care I would say, child drivers, like e.g. this lcd
driver) taking decisions on their own for their resources (even if shared)
or to have parents taking decisions for them? Just asking.

> 
> This may have the nice side-effect that we avoid
> that both the drm driver and the pwm driver enable/disable the periph_clk
> as it is today.
> 
> Another comment - fixed_clksrc is used to determine if sys_clk is enabled.
> But that flag is about the clksource selection, and it is just a coincidence
> that the same flag can be used here.

Yes, agree on this, I took advantage of fixed_clksrc to also enable the
sys_clk based on fixed_clksrc to avoid introducing another member in struct
atmel_hlcdc_dc_desc. At this moment this is valid only for SAM9X60 and I
was thinking that in case some other scenario will appear I will do the
appropriate changes.

> Why we cannot always enable sys_clk? Do we need to do this only
> for sam9x60?

Only SAM9X60 requires this explicitly at probe.

> IF yes, then add a new falg. If no, then skip the flag.
> 
> 	Sam
> 
> 
>>
>> Signed-off-by: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com>
>> [claudiu.beznea@microchip.com: add fixed_clksrc checks]
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> index 0be13eceedba..8bf51f853721 100644
>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
>> @@ -625,10 +625,18 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>  	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
>>  	dev->dev_private = dc;
>>  
>> +	if (dc->desc->fixed_clksrc) {
>> +		ret = clk_prepare_enable(dc->hlcdc->sys_clk);
>> +		if (ret) {
>> +			dev_err(dev->dev, "failed to enable sys_clk\n");
>> +			goto err_destroy_wq;
>> +		}
>> +	}
>> +
>>  	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
>>  	if (ret) {
>>  		dev_err(dev->dev, "failed to enable periph_clk\n");
>> -		goto err_destroy_wq;
>> +		goto err_sys_clk_disable;
>>  	}
>>  
>>  	pm_runtime_enable(dev->dev);
>> @@ -664,6 +672,9 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
>>  err_periph_clk_disable:
>>  	pm_runtime_disable(dev->dev);
>>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
>> +err_sys_clk_disable:
>> +	if (dc->desc->fixed_clksrc)
>> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>>  
>>  err_destroy_wq:
>>  	destroy_workqueue(dc->wq);
>> @@ -688,6 +699,8 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>>  
>>  	pm_runtime_disable(dev->dev);
>>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
>> +	if (dc->desc->fixed_clksrc)
>> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>>  	destroy_workqueue(dc->wq);
>>  }
>>  
>> @@ -805,6 +818,8 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
>>  	regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr);
>>  	regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr);
>>  	clk_disable_unprepare(dc->hlcdc->periph_clk);
>> +	if (dc->desc->fixed_clksrc)
>> +		clk_disable_unprepare(dc->hlcdc->sys_clk);
>>  
>>  	return 0;
>>  }
>> @@ -814,6 +829,8 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev)
>>  	struct drm_device *drm_dev = dev_get_drvdata(dev);
>>  	struct atmel_hlcdc_dc *dc = drm_dev->dev_private;
>>  
>> +	if (dc->desc->fixed_clksrc)
>> +		clk_prepare_enable(dc->hlcdc->sys_clk);
>>  	clk_prepare_enable(dc->hlcdc->periph_clk);
>>  	regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr);
>>  
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

Re: [PATCH 0/7] add LCD support for SAM9X60

[Claudiu.Beznea]

On 28.02.2019 23:13, Sam Ravnborg wrote:
> Hi Alexandre.
> 
>>>>
>>>> These patches adds support for SAM9X60's LCD controller.
>>> Can you elaborate a little more which chips that are relevant.
>>> To be able to look into the right data-sheet, while reviewing.
>>> Link to data-sheet would be nice too.
>>>
>>
>> sam9x60 is the chip name (i.e. x is not a wildcard). I don't think the
>> datasheet is finished/public yet.
> 
> Thanks. I will take a look at the patches but not knowing
> the specifics of the implementation/IP will makes this less valueable.

At this moment, as Alexandre specified, the datasheet is not published.

Thank you,
Claudiu Beznea

> 
> 	Sam
> 

Re: [PATCH 5/7] pwm: atmel-hlcdc: add compatible for SAM9X60 HLCDC's PWM

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 505 bytes --]

On Wed, Feb 27, 2019 at 04:24:35PM +0000, Claudiu.Beznea@microchip.com wrote:
> From: Claudiu Beznea <claudiu.beznea@microchip.com>
> 
> Add compatible string for SAM9X60 HLCDC's PWM.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/pwm/pwm-atmel-hlcdc.c | 3 +++
>  1 file changed, 3 insertions(+)

I guess it makes more sense for this to go in with the MFD changes
rather than through the PWM tree, so:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]