mbox series

[v3,0/6] Exynos-gsc: Support the hardware rotation limits

Message ID 1504850560-27950-1-git-send-email-hoegeun.kwon@samsung.com
Headers show
Series Exynos-gsc: Support the hardware rotation limits | expand

Message

Hoegeun Kwon Sept. 8, 2017, 6:02 a.m. UTC
Hello all,

The gscaler has hardware rotation limits. So this patch set support
the rotate hardware limits of gsc.

To avoid problems with bisectability, patches 1~4 must be merged and
then merged 5 and 6.

Changes for V3:
- Fixed of_match_node() to of_device_get_match_data() in drm gsc driver.
- Added hardware rotation limits for gsc driver of v4l2.
- Added the remove unnecessary compatible for DT document and Exynos dts.

Changes for V2:
- Added the interface info in binding document.
- Added clean name of compatible in Exynos dts.
- Added maximum supported picture size hardcoded into driver.

Best regards,
Hoegeun

Hoegeun Kwon (6):
  [media] exynos-gsc: Add compatible for Exynos 5250 and 5420 specific
    version
  ARM: dts: exynos: Add clean name of compatible.
  drm/exynos/gsc: Add hardware rotation limits
  [media] exynos-gsc: Add hardware rotation limits
  [media] exynos-gsc: Remove unnecessary compatible
  ARM: dts: exynos: Remove unnecessary compatible

 .../devicetree/bindings/media/exynos5-gsc.txt      |  7 +-
 arch/arm/boot/dts/exynos5250.dtsi                  |  8 +-
 arch/arm/boot/dts/exynos5420.dtsi                  |  4 +-
 drivers/gpu/drm/exynos/exynos_drm_gsc.c            | 93 ++++++++++++++-------
 drivers/media/platform/exynos-gsc/gsc-core.c       | 96 +++++++++++++++++++---
 include/uapi/drm/exynos_drm.h                      |  2 +
 6 files changed, 159 insertions(+), 51 deletions(-)

Comments

Robin Murphy Sept. 8, 2017, 11:33 a.m. UTC | #1
On 08/09/17 07:02, Hoegeun Kwon wrote:
> Exynos 5250 and 5420 have different hardware rotation limits. However,
> currently it uses only one compatible - "exynos5-gsc". Since we have
> to distinguish between these two, we add different compatible.
> 
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi | 8 ++++----
>  arch/arm/boot/dts/exynos5420.dtsi | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 8dbeb87..bf08101 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -637,7 +637,7 @@
>  		};
>  
>  		gsc_0:  gsc@13e00000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5250-gsc";

These should be the other way round - the most specific compatible
should come first, then the more general fallback afterwards.

(and similarly in all cases below)

Robin.

>  			reg = <0x13e00000 0x1000>;
>  			interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
>  			power-domains = <&pd_gsc>;
> @@ -647,7 +647,7 @@
>  		};
>  
>  		gsc_1:  gsc@13e10000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5250-gsc";
>  			reg = <0x13e10000 0x1000>;
>  			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>  			power-domains = <&pd_gsc>;
> @@ -657,7 +657,7 @@
>  		};
>  
>  		gsc_2:  gsc@13e20000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5250-gsc";
>  			reg = <0x13e20000 0x1000>;
>  			interrupts = <GIC_SPI 87 IRQ_TYPE_LEVEL_HIGH>;
>  			power-domains = <&pd_gsc>;
> @@ -667,7 +667,7 @@
>  		};
>  
>  		gsc_3:  gsc@13e30000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5250-gsc";
>  			reg = <0x13e30000 0x1000>;
>  			interrupts = <GIC_SPI 88 IRQ_TYPE_LEVEL_HIGH>;
>  			power-domains = <&pd_gsc>;
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index 02d2f89..86afe77 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -658,7 +658,7 @@
>  		};
>  
>  		gsc_0: video-scaler@13e00000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5420-gsc";
>  			reg = <0x13e00000 0x1000>;
>  			interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clock CLK_GSCL0>;
> @@ -668,7 +668,7 @@
>  		};
>  
>  		gsc_1: video-scaler@13e10000 {
> -			compatible = "samsung,exynos5-gsc";
> +			compatible = "samsung,exynos5-gsc", "samsung,exynos5420-gsc";
>  			reg = <0x13e10000 0x1000>;
>  			interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clock CLK_GSCL1>;
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robin Murphy Sept. 8, 2017, 11:58 a.m. UTC | #2
On 08/09/17 07:02, Hoegeun Kwon wrote:
> The gscaler has hardware rotation limits that need to be hardcoded
> into driver. Distinguish them and add them to the property list.
> 
> The hardware rotation limits are related to the cropped source size.
> When swap occurs, use rot_max size instead of crop_max size.
> 
> Also the scaling limits are related to pos size, use pos size to check
> the limits.
> 
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gsc.c | 93 +++++++++++++++++++++++----------
>  include/uapi/drm/exynos_drm.h           |  2 +
>  2 files changed, 66 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gsc.c b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> index 0506b2b..a4fb347 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gsc.c
> @@ -17,6 +17,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> +#include <linux/of_device.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/exynos_drm.h>
> @@ -150,6 +151,15 @@ struct gsc_context {
>  	bool	suspended;
>  };
>  
> +/*
> + * struct gsc_driverdata - per device type driver data for init time.
> + *
> + * @rot_max: rotation max resolution.
> + */
> +struct gsc_driverdata {
> +	struct drm_exynos_sz rot_max;
> +};
> +
>  /* 8-tap Filter Coefficient */
>  static const int h_coef_8t[GSC_COEF_RATIO][GSC_COEF_ATTR][GSC_COEF_H_8T] = {
>  	{	/* Ratio <= 65536 (~8:8) */
> @@ -1401,6 +1411,23 @@ static int gsc_ippdrv_check_property(struct device *dev,
>  	bool swap;
>  	int i;
>  
> +	config = &property->config[EXYNOS_DRM_OPS_DST];
> +
> +	/* check for degree */
> +	switch (config->degree) {
> +	case EXYNOS_DRM_DEGREE_90:
> +	case EXYNOS_DRM_DEGREE_270:
> +		swap = true;
> +		break;
> +	case EXYNOS_DRM_DEGREE_0:
> +	case EXYNOS_DRM_DEGREE_180:
> +		swap = false;
> +		break;
> +	default:
> +		DRM_ERROR("invalid degree.\n");
> +		goto err_property;
> +	}
> +
>  	for_each_ipp_ops(i) {
>  		if ((i == EXYNOS_DRM_OPS_SRC) &&
>  			(property->cmd == IPP_CMD_WB))
> @@ -1416,21 +1443,6 @@ static int gsc_ippdrv_check_property(struct device *dev,
>  			goto err_property;
>  		}
>  
> -		/* check for degree */
> -		switch (config->degree) {
> -		case EXYNOS_DRM_DEGREE_90:
> -		case EXYNOS_DRM_DEGREE_270:
> -			swap = true;
> -			break;
> -		case EXYNOS_DRM_DEGREE_0:
> -		case EXYNOS_DRM_DEGREE_180:
> -			swap = false;
> -			break;
> -		default:
> -			DRM_ERROR("invalid degree.\n");
> -			goto err_property;
> -		}
> -
>  		/* check for buffer bound */
>  		if ((pos->x + pos->w > sz->hsize) ||
>  			(pos->y + pos->h > sz->vsize)) {
> @@ -1438,21 +1450,27 @@ static int gsc_ippdrv_check_property(struct device *dev,
>  			goto err_property;
>  		}
>  
> +		/*
> +		 * The rotation hardware limits are related to the cropped
> +		 * source size. So use rot_max size to check the limits when
> +		 * swap happens. And also the scaling limits are related to pos
> +		 * size, use pos size to check the limits.
> +		 */
>  		/* check for crop */
>  		if ((i == EXYNOS_DRM_OPS_SRC) && (pp->crop)) {
>  			if (swap) {
>  				if ((pos->h < pp->crop_min.hsize) ||
> -					(sz->vsize > pp->crop_max.hsize) ||
> +					(pos->h > pp->rot_max.hsize) ||
>  					(pos->w < pp->crop_min.vsize) ||
> -					(sz->hsize > pp->crop_max.vsize)) {
> +					(pos->w > pp->rot_max.vsize)) {
>  					DRM_ERROR("out of crop size.\n");
>  					goto err_property;
>  				}
>  			} else {
>  				if ((pos->w < pp->crop_min.hsize) ||
> -					(sz->hsize > pp->crop_max.hsize) ||
> +					(pos->w > pp->crop_max.hsize) ||
>  					(pos->h < pp->crop_min.vsize) ||
> -					(sz->vsize > pp->crop_max.vsize)) {
> +					(pos->h > pp->crop_max.vsize)) {
>  					DRM_ERROR("out of crop size.\n");
>  					goto err_property;
>  				}
> @@ -1463,17 +1481,17 @@ static int gsc_ippdrv_check_property(struct device *dev,
>  		if ((i == EXYNOS_DRM_OPS_DST) && (pp->scale)) {
>  			if (swap) {
>  				if ((pos->h < pp->scale_min.hsize) ||
> -					(sz->vsize > pp->scale_max.hsize) ||
> +					(pos->h > pp->scale_max.hsize) ||
>  					(pos->w < pp->scale_min.vsize) ||
> -					(sz->hsize > pp->scale_max.vsize)) {
> +					(pos->w > pp->scale_max.vsize)) {
>  					DRM_ERROR("out of scale size.\n");
>  					goto err_property;
>  				}
>  			} else {
>  				if ((pos->w < pp->scale_min.hsize) ||
> -					(sz->hsize > pp->scale_max.hsize) ||
> +					(pos->w > pp->scale_max.hsize) ||
>  					(pos->h < pp->scale_min.vsize) ||
> -					(sz->vsize > pp->scale_max.vsize)) {
> +					(pos->h > pp->scale_max.vsize)) {
>  					DRM_ERROR("out of scale size.\n");
>  					goto err_property;
>  				}
> @@ -1657,12 +1675,34 @@ static void gsc_ippdrv_stop(struct device *dev, enum drm_exynos_ipp_cmd cmd)
>  	gsc_write(cfg, GSC_ENABLE);
>  }
>  
> +static struct gsc_driverdata gsc_exynos5250_drvdata = {
> +	.rot_max = { 2048, 2048 },
> +};
> +
> +static struct gsc_driverdata gsc_exynos5420_drvdata = {
> +	.rot_max = { 2016, 2016 },
> +};
> +
> +static const struct of_device_id exynos_drm_gsc_of_match[] = {
> +	{
> +		.compatible = "samsung,exynos5250-gsc",
> +		.data = &gsc_exynos5250_drvdata,
> +	},
> +	{
> +		.compatible = "samsung,exynos5420-gsc",
> +		.data = &gsc_exynos5420_drvdata,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, exynos_drm_gsc_of_match);
> +
>  static int gsc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct gsc_context *ctx;
>  	struct resource *res;
>  	struct exynos_drm_ippdrv *ippdrv;
> +	const struct gsc_driverdata *drv_data = of_device_get_match_data(dev);
>  	int ret;
>  
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> @@ -1722,6 +1762,7 @@ static int gsc_probe(struct platform_device *pdev)
>  		dev_err(dev, "failed to init property list.\n");
>  		return ret;
>  	}
> +	ctx->ippdrv.prop_list.rot_max = drv_data->rot_max;
>  
>  	DRM_DEBUG_KMS("id[%d]ippdrv[%pK]\n", ctx->id, ippdrv);
>  
> @@ -1784,12 +1825,6 @@ static int __maybe_unused gsc_runtime_resume(struct device *dev)
>  	SET_RUNTIME_PM_OPS(gsc_runtime_suspend, gsc_runtime_resume, NULL)
>  };
>  
> -static const struct of_device_id exynos_drm_gsc_of_match[] = {
> -	{ .compatible = "samsung,exynos5-gsc" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, exynos_drm_gsc_of_match);

As Krzysztof noted, this breaks compatibility with existing DTBs, and
not everyone wants to update their firmware in lock-step with their
kernel. That said, as far as I can see there's little need to remove it
anyway - as long as there is some set of lowest-common-denominator
limits that will work on any hardware variant, there doesn't seem to be
much harm in keeping a generic fallback around indefinitely.

Robin.

> -
>  struct platform_driver gsc_driver = {
>  	.probe		= gsc_probe,
>  	.remove		= gsc_remove,
> diff --git a/include/uapi/drm/exynos_drm.h b/include/uapi/drm/exynos_drm.h
> index cb3e9f9..d5d5518 100644
> --- a/include/uapi/drm/exynos_drm.h
> +++ b/include/uapi/drm/exynos_drm.h
> @@ -192,6 +192,7 @@ enum drm_exynos_planer {
>   * @crop_max: crop max resolution.
>   * @scale_min: scale min resolution.
>   * @scale_max: scale max resolution.
> + * @rot_max: rotation max resolution.
>   */
>  struct drm_exynos_ipp_prop_list {
>  	__u32	version;
> @@ -210,6 +211,7 @@ struct drm_exynos_ipp_prop_list {
>  	struct drm_exynos_sz	crop_max;
>  	struct drm_exynos_sz	scale_min;
>  	struct drm_exynos_sz	scale_max;
> +	struct drm_exynos_sz	rot_max;
>  };
>  
>  /**
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Sept. 11, 2017, 9:35 a.m. UTC | #3
On 09/08/2017 08:02 AM, Hoegeun Kwon wrote:
> The hardware rotation limits of gsc depends on SOC (Exynos
> 5250/5420/5433). Distinguish them and add them to the driver data.
> 
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> ---
>   drivers/media/platform/exynos-gsc/gsc-core.c | 96 ++++++++++++++++++++++++----
>   1 file changed, 83 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 4380150..8f8636e 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -943,7 +943,37 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
>   	return IRQ_HANDLED;
>   }
>   
> -static struct gsc_pix_max gsc_v_100_max = {
> +static struct gsc_pix_max gsc_v_5250_max = {
> +	.org_scaler_bypass_w	= 8192,
> +	.org_scaler_bypass_h	= 8192,
> +	.org_scaler_input_w	= 4800,
> +	.org_scaler_input_h	= 3344,
> +	.real_rot_dis_w		= 4800,
> +	.real_rot_dis_h		= 3344,
> +	.real_rot_en_w		= 2016,
> +	.real_rot_en_h		= 2016,
> +	.target_rot_dis_w	= 4800,
> +	.target_rot_dis_h	= 3344,
> +	.target_rot_en_w	= 2016,
> +	.target_rot_en_h	= 2016,
> +};
> +
> +static struct gsc_pix_max gsc_v_5420_max = {
> +	.org_scaler_bypass_w	= 8192,
> +	.org_scaler_bypass_h	= 8192,
> +	.org_scaler_input_w	= 4800,
> +	.org_scaler_input_h	= 3344,
> +	.real_rot_dis_w		= 4800,
> +	.real_rot_dis_h		= 3344,
> +	.real_rot_en_w		= 2048,
> +	.real_rot_en_h		= 2048,
> +	.target_rot_dis_w	= 4800,
> +	.target_rot_dis_h	= 3344,
> +	.target_rot_en_w	= 2016,
> +	.target_rot_en_h	= 2016,
> +};
> +
> +static struct gsc_pix_max gsc_v_5433_max = {
>   	.org_scaler_bypass_w	= 8192,
>   	.org_scaler_bypass_h	= 8192,
>   	.org_scaler_input_w	= 4800,
> @@ -979,8 +1009,8 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
>   	.target_h		= 2,  /* yuv420 : 2, others : 1 */
>   };
>   
> -static struct gsc_variant gsc_v_100_variant = {
> -	.pix_max		= &gsc_v_100_max,
> +static struct gsc_variant gsc_v_5250_variant = {
> +	.pix_max		= &gsc_v_5250_max,
>   	.pix_min		= &gsc_v_100_min,
>   	.pix_align		= &gsc_v_100_align,
>   	.in_buf_cnt		= 32,
> @@ -992,12 +1022,48 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
>   	.local_sc_down		= 2,
>   };
>   
> -static struct gsc_driverdata gsc_v_100_drvdata = {
> +static struct gsc_variant gsc_v_5420_variant = {
> +	.pix_max		= &gsc_v_5420_max,
> +	.pix_min		= &gsc_v_100_min,
> +	.pix_align		= &gsc_v_100_align,
> +	.in_buf_cnt		= 32,
> +	.out_buf_cnt		= 32,
> +	.sc_up_max		= 8,
> +	.sc_down_max		= 16,
> +	.poly_sc_down_max	= 4,
> +	.pre_sc_down_max	= 4,
> +	.local_sc_down		= 2,
> +};
> +
> +static struct gsc_variant gsc_v_5433_variant = {
> +	.pix_max		= &gsc_v_5433_max,
> +	.pix_min		= &gsc_v_100_min,
> +	.pix_align		= &gsc_v_100_align,
> +	.in_buf_cnt		= 32,
> +	.out_buf_cnt		= 32,
> +	.sc_up_max		= 8,
> +	.sc_down_max		= 16,
> +	.poly_sc_down_max	= 4,
> +	.pre_sc_down_max	= 4,
> +	.local_sc_down		= 2,
> +};
> +
> +static struct gsc_driverdata gsc_v_5250_drvdata = {
>   	.variant = {
> -		[0] = &gsc_v_100_variant,
> -		[1] = &gsc_v_100_variant,
> -		[2] = &gsc_v_100_variant,
> -		[3] = &gsc_v_100_variant,
> +		[0] = &gsc_v_5250_variant,
> +		[1] = &gsc_v_5250_variant,
> +		[2] = &gsc_v_5250_variant,
> +		[3] = &gsc_v_5250_variant,
> +	},
> +	.num_entities = 4,
> +	.clk_names = { "gscl" },
> +	.num_clocks = 1,
> +};
> +
> +static struct gsc_driverdata gsc_v_5420_drvdata = {
> +	.variant = {
> +		[0] = &gsc_v_5420_variant,
> +		[1] = &gsc_v_5420_variant,
>   	},
>   	.num_entities = 4,
>   	.clk_names = { "gscl" },
> @@ -1006,9 +1072,9 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
>   
>   static struct gsc_driverdata gsc_5433_drvdata = {
>   	.variant = {
> -		[0] = &gsc_v_100_variant,
> -		[1] = &gsc_v_100_variant,
> -		[2] = &gsc_v_100_variant,
> +		[0] = &gsc_v_5433_variant,
> +		[1] = &gsc_v_5433_variant,
> +		[2] = &gsc_v_5433_variant,
>   	},
>   	.num_entities = 3,
>   	.clk_names = { "pclk", "aclk", "aclk_xiu", "aclk_gsclbend" },
> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq, void *priv)
>   
>   static const struct of_device_id exynos_gsc_match[] = {
>   	{
> -		.compatible = "samsung,exynos5-gsc",
> -		.data = &gsc_v_100_drvdata,

Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant
data, so that it can work with "samsung,exynos5-gsc" compatible in DT
on both exynos5250 and exynos5420 SoCs?

> +		.compatible = "samsung,exynos5250-gsc",
> +		.data = &gsc_v_5250_drvdata,
> +	},
> +	{
> +		.compatible = "samsung,exynos5420-gsc",
> +		.data = &gsc_v_5420_drvdata,
>   	},
Hoegeun Kwon Sept. 13, 2017, 2:33 a.m. UTC | #4
Hi Sylwester,

On 09/11/2017 06:35 PM, Sylwester Nawrocki wrote:
> On 09/08/2017 08:02 AM, Hoegeun Kwon wrote:
>> The hardware rotation limits of gsc depends on SOC (Exynos
>> 5250/5420/5433). Distinguish them and add them to the driver data.
>>
>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>> ---
>>   drivers/media/platform/exynos-gsc/gsc-core.c | 96 
>> ++++++++++++++++++++++++----
>>   1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c 
>> b/drivers/media/platform/exynos-gsc/gsc-core.c
>> index 4380150..8f8636e 100644
>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>> @@ -943,7 +943,37 @@ static irqreturn_t gsc_irq_handler(int irq, void 
>> *priv)
>>       return IRQ_HANDLED;
>>   }
>>   -static struct gsc_pix_max gsc_v_100_max = {
>> +static struct gsc_pix_max gsc_v_5250_max = {
>> +    .org_scaler_bypass_w    = 8192,
>> +    .org_scaler_bypass_h    = 8192,
>> +    .org_scaler_input_w    = 4800,
>> +    .org_scaler_input_h    = 3344,
>> +    .real_rot_dis_w        = 4800,
>> +    .real_rot_dis_h        = 3344,
>> +    .real_rot_en_w        = 2016,
>> +    .real_rot_en_h        = 2016,
>> +    .target_rot_dis_w    = 4800,
>> +    .target_rot_dis_h    = 3344,
>> +    .target_rot_en_w    = 2016,
>> +    .target_rot_en_h    = 2016,
>> +};
>> +
>> +static struct gsc_pix_max gsc_v_5420_max = {
>> +    .org_scaler_bypass_w    = 8192,
>> +    .org_scaler_bypass_h    = 8192,
>> +    .org_scaler_input_w    = 4800,
>> +    .org_scaler_input_h    = 3344,
>> +    .real_rot_dis_w        = 4800,
>> +    .real_rot_dis_h        = 3344,
>> +    .real_rot_en_w        = 2048,
>> +    .real_rot_en_h        = 2048,
>> +    .target_rot_dis_w    = 4800,
>> +    .target_rot_dis_h    = 3344,
>> +    .target_rot_en_w    = 2016,
>> +    .target_rot_en_h    = 2016,
>> +};
>> +
>> +static struct gsc_pix_max gsc_v_5433_max = {
>>       .org_scaler_bypass_w    = 8192,
>>       .org_scaler_bypass_h    = 8192,
>>       .org_scaler_input_w    = 4800,
>> @@ -979,8 +1009,8 @@ static irqreturn_t gsc_irq_handler(int irq, void 
>> *priv)
>>       .target_h        = 2,  /* yuv420 : 2, others : 1 */
>>   };
>>   -static struct gsc_variant gsc_v_100_variant = {
>> -    .pix_max        = &gsc_v_100_max,
>> +static struct gsc_variant gsc_v_5250_variant = {
>> +    .pix_max        = &gsc_v_5250_max,
>>       .pix_min        = &gsc_v_100_min,
>>       .pix_align        = &gsc_v_100_align,
>>       .in_buf_cnt        = 32,
>> @@ -992,12 +1022,48 @@ static irqreturn_t gsc_irq_handler(int irq, 
>> void *priv)
>>       .local_sc_down        = 2,
>>   };
>>   -static struct gsc_driverdata gsc_v_100_drvdata = {
>> +static struct gsc_variant gsc_v_5420_variant = {
>> +    .pix_max        = &gsc_v_5420_max,
>> +    .pix_min        = &gsc_v_100_min,
>> +    .pix_align        = &gsc_v_100_align,
>> +    .in_buf_cnt        = 32,
>> +    .out_buf_cnt        = 32,
>> +    .sc_up_max        = 8,
>> +    .sc_down_max        = 16,
>> +    .poly_sc_down_max    = 4,
>> +    .pre_sc_down_max    = 4,
>> +    .local_sc_down        = 2,
>> +};
>> +
>> +static struct gsc_variant gsc_v_5433_variant = {
>> +    .pix_max        = &gsc_v_5433_max,
>> +    .pix_min        = &gsc_v_100_min,
>> +    .pix_align        = &gsc_v_100_align,
>> +    .in_buf_cnt        = 32,
>> +    .out_buf_cnt        = 32,
>> +    .sc_up_max        = 8,
>> +    .sc_down_max        = 16,
>> +    .poly_sc_down_max    = 4,
>> +    .pre_sc_down_max    = 4,
>> +    .local_sc_down        = 2,
>> +};
>> +
>> +static struct gsc_driverdata gsc_v_5250_drvdata = {
>>       .variant = {
>> -        [0] = &gsc_v_100_variant,
>> -        [1] = &gsc_v_100_variant,
>> -        [2] = &gsc_v_100_variant,
>> -        [3] = &gsc_v_100_variant,
>> +        [0] = &gsc_v_5250_variant,
>> +        [1] = &gsc_v_5250_variant,
>> +        [2] = &gsc_v_5250_variant,
>> +        [3] = &gsc_v_5250_variant,
>> +    },
>> +    .num_entities = 4,
>> +    .clk_names = { "gscl" },
>> +    .num_clocks = 1,
>> +};
>> +
>> +static struct gsc_driverdata gsc_v_5420_drvdata = {
>> +    .variant = {
>> +        [0] = &gsc_v_5420_variant,
>> +        [1] = &gsc_v_5420_variant,
>>       },
>>       .num_entities = 4,
>>       .clk_names = { "gscl" },
>> @@ -1006,9 +1072,9 @@ static irqreturn_t gsc_irq_handler(int irq, 
>> void *priv)
>>     static struct gsc_driverdata gsc_5433_drvdata = {
>>       .variant = {
>> -        [0] = &gsc_v_100_variant,
>> -        [1] = &gsc_v_100_variant,
>> -        [2] = &gsc_v_100_variant,
>> +        [0] = &gsc_v_5433_variant,
>> +        [1] = &gsc_v_5433_variant,
>> +        [2] = &gsc_v_5433_variant,
>>       },
>>       .num_entities = 3,
>>       .clk_names = { "pclk", "aclk", "aclk_xiu", "aclk_gsclbend" },
>> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq, 
>> void *priv)
>>     static const struct of_device_id exynos_gsc_match[] = {
>>       {
>> -        .compatible = "samsung,exynos5-gsc",
>> -        .data = &gsc_v_100_drvdata,
>
> Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant
> data, so that it can work with "samsung,exynos5-gsc" compatible in DT
> on both exynos5250 and exynos5420 SoCs?
>

Thank you for your question.

Exynos 5250 and 5420 have different hardware rotation limits.
Exynos 5250 is '.real_rot_en_w/h = 2016' and 5420 is '.real_rot_en_w/h = 
2048'.

So my opinion they must have different compatible.

Best regards,
Hoegeun

>> +        .compatible = "samsung,exynos5250-gsc",
>> +        .data = &gsc_v_5250_drvdata,
>> +    },
>> +    {
>> +        .compatible = "samsung,exynos5420-gsc",
>> +        .data = &gsc_v_5420_drvdata,
>>       },
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sylwester Nawrocki Sept. 13, 2017, 9:11 a.m. UTC | #5
Hi Hoegeun,

On 09/13/2017 04:33 AM, Hoegeun Kwon wrote:
>>> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq,
>>> void *priv)
>>>      static const struct of_device_id exynos_gsc_match[] = {
>>>        {
>>> -        .compatible = "samsung,exynos5-gsc",
>>> -        .data = &gsc_v_100_drvdata,
>> Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant
>> data, so that it can work with "samsung,exynos5-gsc" compatible in DT
>> on both exynos5250 and exynos5420 SoCs?
>>
> Thank you for your question.
> 
> Exynos 5250 and 5420 have different hardware rotation limits.
> Exynos 5250 is '.real_rot_en_w/h = 2016' and 5420 is '.real_rot_en_w/h =
> 2048'.
> 
> So my opinion they must have different compatible.

I think there is some misunderstanding, mu suggestion was to keep the 
"samsung,exynos5-gsc" compatible entry in addition to the new introduced 
ones: "samsung,exynos5250-gsc" and "samsung,exynos5420-gsc". That's in
order to make your changes possibly backward compatible, in theory 
the updated driver should still work without changes in dts.
Hoegeun Kwon Sept. 13, 2017, 9:28 a.m. UTC | #6
On 09/13/2017 06:11 PM, Sylwester Nawrocki wrote:
> Hi Hoegeun,
>
> On 09/13/2017 04:33 AM, Hoegeun Kwon wrote:
>>>> @@ -1017,8 +1083,12 @@ static irqreturn_t gsc_irq_handler(int irq,
>>>> void *priv)
>>>>       static const struct of_device_id exynos_gsc_match[] = {
>>>>         {
>>>> -        .compatible = "samsung,exynos5-gsc",
>>>> -        .data = &gsc_v_100_drvdata,
>>> Can you keep the "samsung,exynos5-gsc" entry with the gsc_v_5250_variant
>>> data, so that it can work with "samsung,exynos5-gsc" compatible in DT
>>> on both exynos5250 and exynos5420 SoCs?
>>>
>> Thank you for your question.
>>
>> Exynos 5250 and 5420 have different hardware rotation limits.
>> Exynos 5250 is '.real_rot_en_w/h = 2016' and 5420 is '.real_rot_en_w/h =
>> 2048'.
>>
>> So my opinion they must have different compatible.
> I think there is some misunderstanding, mu suggestion was to keep the
> "samsung,exynos5-gsc" compatible entry in addition to the new introduced
> ones: "samsung,exynos5250-gsc" and "samsung,exynos5420-gsc". That's in
> order to make your changes possibly backward compatible, in theory
> the updated driver should still work without changes in dts.


Thank you again for your explanation.

Yes, I understood.
I will keep "samsung,exynos5-gsc" compatible,
and add Exynos 5250/5420/5433 compatible.

Best regards,
Hoegeun

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html