* Re: [PATCH 2/3] sdhci: sparx5: Add Sparx5 SoC eMMC driver
From: Adrian Hunter @ 2020-05-24 19:26 UTC (permalink / raw)
To: Lars Povlsen
Cc: Ulf Hansson, SoC Team, Microchip Linux Driver Support, linux-mmc,
devicetree, linux-arm-kernel, linux-kernel, Alexandre Belloni
In-Reply-To: <87wo56q2o3.fsf@soft-dev15.microsemi.net>
On 20/05/20 2:14 pm, Lars Povlsen wrote:
>
> Lars Povlsen writes:
>
>> Adrian Hunter writes:
>>
>>> On 13/05/20 4:31 pm, Lars Povlsen wrote:
>>>> This adds the eMMC driver for the Sparx5 SoC. It is based upon the
>>>> designware IP, but requires some extra initialization and quirks.
>>>>
>>>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>>>> ---
> {Snip]
>>>> +};
>>>> +
>>>> +static const struct sdhci_pltfm_data sdhci_sparx5_pdata = {
>>>> + .quirks = 0,
>>>> + .quirks2 = SDHCI_QUIRK2_HOST_NO_CMD23 | /* Card quirk */
>>>
>>> If this is a card quirk then it should be in drivers/mmc/core/quirks.h not here.
>>
>
> Adrian, I had a go at changing the controller quirk to a card quirk.
>
> Unfortunately, SDHCI_QUIRK2_HOST_NO_CMD23 does not directly translate to
> MMC_QUIRK_BLK_NO_CMD23, as for 'do_rel_wr' in mmc_blk_rw_rq_prep(), it
> will *still* use MMC_SET_BLOCK_COUNT (cmd23), causing the issue.
>
> We are using a ISSI "IS004G" device, and so I have gone through the
> motions of adding it to quirks.h. The comment before the list of devices
> using MMC_QUIRK_BLK_NO_CMD23 suggest working around a performance issue,
> which is not exactly the issue I'm seeing. I'm seeing combinations of
> CMD_TOUT_ERR, DATA_CRC_ERR and DATA_END_BIT_ERR whenever a cmd23 is
> issued.
>
> I have not been able to test the controller with another eMMC device
> yet, but I expect its not the controller at fault.
>
> So, I'm a little bit in doubt of how to proceed - either keep the quirk
> as a controller quirk - or make a *new* card quirk (with
> SDHCI_QUIRK2_HOST_NO_CMD23 semantics)?
>
> Anybody else have had experience with ISSI eMMC devices?
>
> I have also tried to use DT sdhci-caps-mask, but MMC_CAP_CMD23 is not
> read from the controller just (unconditionally) set in sdhci.c - so that
> doesn't fly either.
>
> Any suggestions?
It is up to you. In the future, you may want to distinguish devices that
have this problem from ones that do not.
If you are not sure it is the ISSI eMMC, and maybe not the host controller,
then might it be the board? Perhaps make SDHCI_QUIRK2_HOST_NO_CMD23
conditional on the particular compatibility string?
At a minimum, change the "/* Card quirk */" comment to a fuller explanation.
>
>> Yes, its supposedly a card quirk. I'll see to use the card quirks
>> methods in place.
>>
>
^ permalink raw reply
* [PATCH v3 04/10] media: i2c: imx290: Add support for 2 data lanes
From: Andrey Konovalov @ 2020-05-24 19:24 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
The IMX290 sensor can output frames with 2/4 CSI2 data lanes. This commit
adds support for 2 lane mode in addition to the 4 lane and also
configuring the data lane settings in the driver based on system
configuration.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 133 ++++++++++++++++++++++++++++++++++---
1 file changed, 124 insertions(+), 9 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 7b1de1f0c8b7..a361c9ac8bd5 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -25,7 +25,18 @@
#define IMX290_STANDBY 0x3000
#define IMX290_REGHOLD 0x3001
#define IMX290_XMSTA 0x3002
+#define IMX290_FR_FDG_SEL 0x3009
#define IMX290_GAIN 0x3014
+#define IMX290_HMAX_LOW 0x301c
+#define IMX290_HMAX_HIGH 0x301d
+#define IMX290_PHY_LANE_NUM 0x3407
+#define IMX290_CSI_LANE_MODE 0x3443
+
+/* HMAX fields */
+#define IMX290_HMAX_2_1920 0x1130
+#define IMX290_HMAX_4_1920 0x0898
+#define IMX290_HMAX_2_720 0x19C8
+#define IMX290_HMAX_4_720 0x0CE4
#define IMX290_DEFAULT_LINK_FREQ 445500000
@@ -56,6 +67,7 @@ struct imx290 {
struct device *dev;
struct clk *xclk;
struct regmap *regmap;
+ u8 nlanes;
struct v4l2_subdev sd;
struct v4l2_fwnode_endpoint ep;
@@ -89,14 +101,11 @@ static const struct regmap_config imx290_regmap_config = {
static const struct imx290_regval imx290_global_init_settings[] = {
{ 0x3007, 0x00 },
- { 0x3009, 0x00 },
{ 0x3018, 0x65 },
{ 0x3019, 0x04 },
{ 0x301a, 0x00 },
- { 0x3443, 0x03 },
{ 0x3444, 0x20 },
{ 0x3445, 0x25 },
- { 0x3407, 0x03 },
{ 0x303a, 0x0c },
{ 0x3040, 0x00 },
{ 0x3041, 0x00 },
@@ -169,7 +178,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
{ 0x3164, 0x1a },
{ 0x3480, 0x49 },
/* data rate settings */
- { 0x3009, 0x01 },
{ 0x3405, 0x10 },
{ 0x3446, 0x57 },
{ 0x3447, 0x00 },
@@ -187,8 +195,6 @@ static const struct imx290_regval imx290_1080p_settings[] = {
{ 0x3453, 0x00 },
{ 0x3454, 0x17 },
{ 0x3455, 0x00 },
- { 0x301c, 0x98 },
- { 0x301d, 0x08 },
};
static const struct imx290_regval imx290_720p_settings[] = {
@@ -210,7 +216,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
{ 0x3164, 0x1a },
{ 0x3480, 0x49 },
/* data rate settings */
- { 0x3009, 0x01 },
{ 0x3405, 0x10 },
{ 0x3446, 0x4f },
{ 0x3447, 0x00 },
@@ -228,8 +233,6 @@ static const struct imx290_regval imx290_720p_settings[] = {
{ 0x3453, 0x00 },
{ 0x3454, 0x17 },
{ 0x3455, 0x00 },
- { 0x301c, 0xe4 },
- { 0x301d, 0x0c },
};
static const struct imx290_regval imx290_10bit_settings[] = {
@@ -522,6 +525,25 @@ static int imx290_write_current_format(struct imx290 *imx290,
return 0;
}
+static int imx290_set_hmax(struct imx290 *imx290, u32 val)
+{
+ int ret;
+
+ ret = imx290_write_reg(imx290, IMX290_HMAX_LOW, (val & 0xff));
+ if (ret) {
+ dev_err(imx290->dev, "Error setting HMAX register\n");
+ return ret;
+ }
+
+ ret = imx290_write_reg(imx290, IMX290_HMAX_HIGH, ((val >> 8) & 0xff));
+ if (ret) {
+ dev_err(imx290->dev, "Error setting HMAX register\n");
+ return ret;
+ }
+
+ return 0;
+}
+
/* Start streaming */
static int imx290_start_streaming(struct imx290 *imx290)
{
@@ -551,6 +573,40 @@ static int imx290_start_streaming(struct imx290 *imx290)
return ret;
}
+ switch (imx290->nlanes) {
+ case 2:
+ if (imx290->current_mode->width == 1920) {
+ ret = imx290_set_hmax(imx290, IMX290_HMAX_2_1920);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = imx290_set_hmax(imx290, IMX290_HMAX_2_720);
+ if (ret < 0)
+ return ret;
+ }
+
+ break;
+ case 4:
+ if (imx290->current_mode->width == 1920) {
+ ret = imx290_set_hmax(imx290, IMX290_HMAX_4_1920);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = imx290_set_hmax(imx290, IMX290_HMAX_4_720);
+ if (ret < 0)
+ return ret;
+ }
+
+ break;
+ default:
+ /*
+ * We should never hit this since the data lane count is
+ * validated in probe itself
+ */
+ dev_err(imx290->dev, "Lane configuration not supported\n");
+ return -EINVAL;
+ }
+
/* Apply customized values from user */
ret = v4l2_ctrl_handler_setup(imx290->sd.ctrl_handler);
if (ret) {
@@ -607,6 +663,49 @@ static int imx290_get_regulators(struct device *dev, struct imx290 *imx290)
imx290->supplies);
}
+static int imx290_set_data_lanes(struct imx290 *imx290)
+{
+ int ret = 0, laneval, frsel;
+
+ switch (imx290->nlanes) {
+ case 2:
+ laneval = 0x01;
+ frsel = 0x02;
+ break;
+ case 4:
+ laneval = 0x03;
+ frsel = 0x01;
+ break;
+ default:
+ /*
+ * We should never hit this since the data lane count is
+ * validated in probe itself
+ */
+ dev_err(imx290->dev, "Lane configuration not supported\n");
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
+ if (ret) {
+ dev_err(imx290->dev, "Error setting Physical Lane number register\n");
+ goto exit;
+ }
+
+ ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
+ if (ret) {
+ dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
+ goto exit;
+ }
+
+ ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
+ if (ret)
+ dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
+
+exit:
+ return ret;
+}
+
static int imx290_power_on(struct device *dev)
{
struct i2c_client *client = to_i2c_client(dev);
@@ -631,6 +730,9 @@ static int imx290_power_on(struct device *dev)
gpiod_set_value_cansleep(imx290->rst_gpio, 0);
usleep_range(30000, 31000);
+ /* Set data lane count */
+ imx290_set_data_lanes(imx290);
+
return 0;
}
@@ -703,6 +805,16 @@ static int imx290_probe(struct i2c_client *client)
goto free_err;
}
+ /* Get number of data lanes */
+ imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
+ if (imx290->nlanes != 2 && imx290->nlanes != 4) {
+ dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
+ ret = -EINVAL;
+ goto free_err;
+ }
+
+ dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
+
if (!imx290->ep.nr_of_link_frequencies) {
dev_err(dev, "link-frequency property not found in DT\n");
ret = -EINVAL;
@@ -823,6 +935,9 @@ static int imx290_probe(struct i2c_client *client)
goto free_entity;
}
+ /* Set data lane count */
+ imx290_set_data_lanes(imx290);
+
pm_runtime_set_active(dev);
pm_runtime_enable(dev);
pm_runtime_idle(dev);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 07/10] media: i2c: imx290: Add RAW12 mode support
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
IMX290 is capable of outputting frames in both Raw Bayer (packed) 10 and
12 bit formats. Since the driver already supports RAW10 mode, let's add
the missing RAW12 mode as well.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 36 +++++++++++++++++++++++++++++++++---
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 162c345fffac..6e70ff22bc5f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -71,6 +71,7 @@ struct imx290 {
struct clk *xclk;
struct regmap *regmap;
u8 nlanes;
+ u8 bpp;
struct v4l2_subdev sd;
struct v4l2_fwnode_endpoint ep;
@@ -90,10 +91,12 @@ struct imx290 {
struct imx290_pixfmt {
u32 code;
+ u8 bpp;
};
static const struct imx290_pixfmt imx290_formats[] = {
- { MEDIA_BUS_FMT_SRGGB10_1X10 },
+ { MEDIA_BUS_FMT_SRGGB10_1X10, 10 },
+ { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
};
static const struct regmap_config imx290_regmap_config = {
@@ -261,6 +264,18 @@ static const struct imx290_regval imx290_10bit_settings[] = {
{ 0x300b, 0x00},
};
+static const struct imx290_regval imx290_12bit_settings[] = {
+ { 0x3005, 0x01 },
+ { 0x3046, 0x01 },
+ { 0x3129, 0x00 },
+ { 0x317c, 0x00 },
+ { 0x31ec, 0x0e },
+ { 0x3441, 0x0c },
+ { 0x3442, 0x0c },
+ { 0x300a, 0xf0 },
+ { 0x300b, 0x00 },
+};
+
/* supported link frequencies */
static const s64 imx290_link_freq_2lanes[] = {
891000000, /* 1920x1080 - 2 lane */
@@ -421,7 +436,12 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
} else {
imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
msleep(10);
- imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
+ if (imx290->bpp == 10)
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+ 0x3c);
+ else /* 12 bits per pixel */
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW,
+ 0xf0);
imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
}
break;
@@ -496,7 +516,7 @@ static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
u8 nlanes = imx290->nlanes;
/* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
- return (link_freq * 2 * nlanes / 10);
+ return (link_freq * 2 * nlanes / imx290->bpp);
}
static int imx290_set_fmt(struct v4l2_subdev *sd,
@@ -533,6 +553,7 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
} else {
format = &imx290->current_format;
imx290->current_mode = mode;
+ imx290->bpp = imx290_formats[i].bpp;
if (imx290->link_freq)
__v4l2_ctrl_s_ctrl(imx290->link_freq,
@@ -577,6 +598,15 @@ static int imx290_write_current_format(struct imx290 *imx290)
return ret;
}
break;
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ ret = imx290_set_register_array(imx290, imx290_12bit_settings,
+ ARRAY_SIZE(
+ imx290_12bit_settings));
+ if (ret < 0) {
+ dev_err(imx290->dev, "Could not set format registers\n");
+ return ret;
+ }
+ break;
default:
dev_err(imx290->dev, "Unknown pixel format\n");
return -EINVAL;
--
2.17.1
^ permalink raw reply related
* [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Add support to enumerate all frame sizes supported by IMX290. This is
required for using with userspace tools such as libcamera.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 6e70ff22bc5f..88850f3b1427 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -471,6 +471,25 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
return 0;
}
+static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_pad_config *cfg,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ if ((fse->code != imx290_formats[0].code) &&
+ (fse->code != imx290_formats[1].code))
+ return -EINVAL;
+
+ if (fse->index >= ARRAY_SIZE(imx290_modes))
+ return -EINVAL;
+
+ fse->min_width = imx290_modes[fse->index].width;
+ fse->max_width = imx290_modes[fse->index].width;
+ fse->min_height = imx290_modes[fse->index].height;
+ fse->max_height = imx290_modes[fse->index].height;
+
+ return 0;
+}
+
static int imx290_get_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *fmt)
@@ -850,6 +869,7 @@ static const struct v4l2_subdev_video_ops imx290_video_ops = {
static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
.init_cfg = imx290_entity_init_cfg,
.enum_mbus_code = imx290_enum_mbus_code,
+ .enum_frame_size = imx290_enum_frame_size,
.get_fmt = imx290_get_fmt,
.set_fmt = imx290_set_fmt,
};
--
2.17.1
^ permalink raw reply related
* [PATCH v3 10/10] media: i2c: imx290: set bus_type before calling v4l2_fwnode_endpoint_alloc_parse()
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
The bus_type field of v4l2_fwnode_endpoint structure passed as the argument
to v4l2_fwnode_endpoint_alloc_parse() function must be initiaized.
Set it to V4L2_MBUS_CSI2_DPHY, and check for -ENXIO which is returned
when the requested media bus type doesn't match the fwnode.
Also remove v4l2_fwnode_endpoint field from struct imx290 as it is only
needed in the probe function: use the local variable for this purpose.
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index ee5c95cf64f3..05a3d897614e 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -74,7 +74,6 @@ struct imx290 {
u8 bpp;
struct v4l2_subdev sd;
- struct v4l2_fwnode_endpoint ep;
struct media_pad pad;
struct v4l2_mbus_framefmt current_format;
const struct imx290_mode *current_mode;
@@ -887,6 +886,10 @@ static int imx290_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
struct fwnode_handle *endpoint;
+ /* Only CSI2 is supported for now: */
+ struct v4l2_fwnode_endpoint ep = {
+ .bus_type = V4L2_MBUS_CSI2_DPHY
+ };
struct imx290 *imx290;
u32 xclk_freq;
int ret;
@@ -908,15 +911,18 @@ static int imx290_probe(struct i2c_client *client)
return -EINVAL;
}
- ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &imx290->ep);
+ ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep);
fwnode_handle_put(endpoint);
- if (ret) {
+ if (ret == -ENXIO) {
+ dev_err(dev, "Unsupported bus type, should be CSI2\n");
+ goto free_err;
+ } else if (ret) {
dev_err(dev, "Parsing endpoint node failed\n");
goto free_err;
}
/* Get number of data lanes */
- imx290->nlanes = imx290->ep.bus.mipi_csi2.num_data_lanes;
+ imx290->nlanes = ep.bus.mipi_csi2.num_data_lanes;
if (imx290->nlanes != 2 && imx290->nlanes != 4) {
dev_err(dev, "Invalid data lanes: %d\n", imx290->nlanes);
ret = -EINVAL;
@@ -925,19 +931,12 @@ static int imx290_probe(struct i2c_client *client)
dev_dbg(dev, "Using %u data lanes\n", imx290->nlanes);
- if (!imx290->ep.nr_of_link_frequencies) {
+ if (!ep.nr_of_link_frequencies) {
dev_err(dev, "link-frequency property not found in DT\n");
ret = -EINVAL;
goto free_err;
}
- /* Only CSI2 is supported for now */
- if (imx290->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
- dev_err(dev, "Unsupported bus type, should be CSI2\n");
- ret = -EINVAL;
- goto free_err;
- }
-
/* get system clock (xclk) */
imx290->xclk = devm_clk_get(dev, "xclk");
if (IS_ERR(imx290->xclk)) {
@@ -1063,7 +1062,7 @@ static int imx290_probe(struct i2c_client *client)
pm_runtime_enable(dev);
pm_runtime_idle(dev);
- v4l2_fwnode_endpoint_free(&imx290->ep);
+ v4l2_fwnode_endpoint_free(&ep);
return 0;
@@ -1073,7 +1072,7 @@ static int imx290_probe(struct i2c_client *client)
v4l2_ctrl_handler_free(&imx290->ctrls);
mutex_destroy(&imx290->lock);
free_err:
- v4l2_fwnode_endpoint_free(&imx290->ep);
+ v4l2_fwnode_endpoint_free(&ep);
return ret;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 09/10] media: i2c: imx290: Move the settle time delay out of loop
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
The 10ms settle time is needed only at the end of all consecutive
register writes. So move the delay to outside of the for loop of
imx290_set_register_array().
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 88850f3b1427..ee5c95cf64f3 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -347,11 +347,11 @@ static int imx290_set_register_array(struct imx290 *imx290,
ret = imx290_write_reg(imx290, settings->reg, settings->val);
if (ret < 0)
return ret;
-
- /* Settle time is 10ms for all registers */
- msleep(10);
}
+ /* Provide 10ms settle time */
+ msleep(10);
+
return 0;
}
--
2.17.1
^ permalink raw reply related
* [PATCH v3 06/10] media: i2c: imx290: Add support for test pattern generation
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Add support for generating following test patterns by IMX290:
* Sequence Pattern 1
* Horizontal Color-bar Chart
* Vertical Color-bar Chart
* Sequence Pattern 2
* Gradation Pattern 1
* Gradation Pattern 2
* 000/555h Toggle Pattern
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 41 +++++++++++++++++++++++++++++++++++++-
1 file changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index e800557cf423..162c345fffac 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -26,12 +26,19 @@
#define IMX290_REGHOLD 0x3001
#define IMX290_XMSTA 0x3002
#define IMX290_FR_FDG_SEL 0x3009
+#define IMX290_BLKLEVEL_LOW 0x300a
+#define IMX290_BLKLEVEL_HIGH 0x300b
#define IMX290_GAIN 0x3014
#define IMX290_HMAX_LOW 0x301c
#define IMX290_HMAX_HIGH 0x301d
+#define IMX290_PGCTRL 0x308c
#define IMX290_PHY_LANE_NUM 0x3407
#define IMX290_CSI_LANE_MODE 0x3443
+#define IMX290_PGCTRL_REGEN BIT(0)
+#define IMX290_PGCTRL_THRU BIT(1)
+#define IMX290_PGCTRL_MODE(n) ((n) << 4)
+
/* HMAX fields */
#define IMX290_HMAX_2_1920 0x1130
#define IMX290_HMAX_4_1920 0x0898
@@ -95,6 +102,17 @@ static const struct regmap_config imx290_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
+static const char * const imx290_test_pattern_menu[] = {
+ "Disabled",
+ "Sequence Pattern 1",
+ "Horizontal Color-bar Chart",
+ "Vertical Color-bar Chart",
+ "Sequence Pattern 2",
+ "Gradation Pattern 1",
+ "Gradation Pattern 2",
+ "000/555h Toggle Pattern",
+};
+
static const struct imx290_regval imx290_global_init_settings[] = {
{ 0x3007, 0x00 },
{ 0x3018, 0x65 },
@@ -391,6 +409,22 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
case V4L2_CID_GAIN:
ret = imx290_set_gain(imx290, ctrl->val);
break;
+ case V4L2_CID_TEST_PATTERN:
+ if (ctrl->val) {
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x00);
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+ msleep(10);
+ imx290_write_reg(imx290, IMX290_PGCTRL,
+ (u8)(IMX290_PGCTRL_REGEN |
+ IMX290_PGCTRL_THRU |
+ IMX290_PGCTRL_MODE(ctrl->val)));
+ } else {
+ imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
+ msleep(10);
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_LOW, 0x3c);
+ imx290_write_reg(imx290, IMX290_BLKLEVEL_HIGH, 0x00);
+ }
+ break;
default:
ret = -EINVAL;
break;
@@ -906,7 +940,7 @@ static int imx290_probe(struct i2c_client *client)
*/
imx290_entity_init_cfg(&imx290->sd, NULL);
- v4l2_ctrl_handler_init(&imx290->ctrls, 3);
+ v4l2_ctrl_handler_init(&imx290->ctrls, 4);
v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_GAIN, 0, 72, 1, 0);
@@ -932,6 +966,11 @@ static int imx290_probe(struct i2c_client *client)
INT_MAX, 1,
imx290_calc_pixel_rate(imx290));
+ v4l2_ctrl_new_std_menu_items(&imx290->ctrls, &imx290_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(imx290_test_pattern_menu) - 1,
+ 0, 0, imx290_test_pattern_menu);
+
imx290->sd.ctrl_handler = &imx290->ctrls;
if (imx290->ctrls.error) {
--
2.17.1
^ permalink raw reply related
* [PATCH v3 05/10] media: i2c: imx290: Add configurable link frequency and pixel rate
From: Andrey Konovalov @ 2020-05-24 19:25 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
IMX290 operates with multiple link frequency and pixel rate combinations.
The initial driver used a single setting for both but since we now have
the lane count support in place, let's add configurable link frequency
and pixel rate.
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
drivers/media/i2c/imx290.c | 100 ++++++++++++++++++++++++-------------
1 file changed, 66 insertions(+), 34 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index a361c9ac8bd5..e800557cf423 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -38,8 +38,6 @@
#define IMX290_HMAX_2_720 0x19C8
#define IMX290_HMAX_4_720 0x0CE4
-#define IMX290_DEFAULT_LINK_FREQ 445500000
-
static const char * const imx290_supply_name[] = {
"vdda",
"vddd",
@@ -56,8 +54,6 @@ struct imx290_regval {
struct imx290_mode {
u32 width;
u32 height;
- u32 pixel_rate;
- u32 link_freq_index;
const struct imx290_regval *data;
u32 data_size;
@@ -248,8 +244,13 @@ static const struct imx290_regval imx290_10bit_settings[] = {
};
/* supported link frequencies */
-static const s64 imx290_link_freq[] = {
- IMX290_DEFAULT_LINK_FREQ,
+static const s64 imx290_link_freq_2lanes[] = {
+ 891000000, /* 1920x1080 - 2 lane */
+ 594000000, /* 1280x720 - 2 lane */
+};
+static const s64 imx290_link_freq_4lanes[] = {
+ 445500000, /* 1920x1080 - 4 lane */
+ 297000000, /* 1280x720 - 4 lane */
};
/* Mode configs */
@@ -259,16 +260,12 @@ static const struct imx290_mode imx290_modes[] = {
.height = 1080,
.data = imx290_1080p_settings,
.data_size = ARRAY_SIZE(imx290_1080p_settings),
- .pixel_rate = 178200000,
- .link_freq_index = 0,
},
{
.width = 1280,
.height = 720,
.data = imx290_720p_settings,
.data_size = ARRAY_SIZE(imx290_720p_settings),
- .pixel_rate = 178200000,
- .link_freq_index = 0,
},
};
@@ -442,6 +439,32 @@ static int imx290_get_fmt(struct v4l2_subdev *sd,
return 0;
}
+static u8 imx290_get_link_freq_index(struct imx290 *imx290)
+{
+ const struct imx290_mode *cur_mode = imx290->current_mode;
+
+ return (cur_mode->width == 1920) ? 0 : 1;
+}
+
+static s64 imx290_get_link_freq(struct imx290 *imx290)
+{
+ u8 index = imx290_get_link_freq_index(imx290);
+
+ if (imx290->nlanes == 4)
+ return imx290_link_freq_4lanes[index];
+ else
+ return imx290_link_freq_2lanes[index];
+}
+
+static u64 imx290_calc_pixel_rate(struct imx290 *imx290)
+{
+ s64 link_freq = imx290_get_link_freq(imx290);
+ u8 nlanes = imx290->nlanes;
+
+ /* pixel rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
+ return (link_freq * 2 * nlanes / 10);
+}
+
static int imx290_set_fmt(struct v4l2_subdev *sd,
struct v4l2_subdev_pad_config *cfg,
struct v4l2_subdev_format *fmt)
@@ -475,10 +498,14 @@ static int imx290_set_fmt(struct v4l2_subdev *sd,
format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
} else {
format = &imx290->current_format;
- __v4l2_ctrl_s_ctrl(imx290->link_freq, mode->link_freq_index);
- __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate, mode->pixel_rate);
-
imx290->current_mode = mode;
+
+ if (imx290->link_freq)
+ __v4l2_ctrl_s_ctrl(imx290->link_freq,
+ imx290_get_link_freq_index(imx290));
+ if (imx290->pixel_rate)
+ __v4l2_ctrl_s_ctrl_int64(imx290->pixel_rate,
+ imx290_calc_pixel_rate(imx290));
}
*format = fmt->format;
@@ -502,12 +529,11 @@ static int imx290_entity_init_cfg(struct v4l2_subdev *subdev,
return 0;
}
-static int imx290_write_current_format(struct imx290 *imx290,
- struct v4l2_mbus_framefmt *format)
+static int imx290_write_current_format(struct imx290 *imx290)
{
int ret;
- switch (format->code) {
+ switch (imx290->current_format.code) {
case MEDIA_BUS_FMT_SRGGB10_1X10:
ret = imx290_set_register_array(imx290, imx290_10bit_settings,
ARRAY_SIZE(
@@ -558,8 +584,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
return ret;
}
- /* Set current frame format */
- ret = imx290_write_current_format(imx290, &imx290->current_format);
+ /* Apply the register values related to current frame format */
+ ret = imx290_write_current_format(imx290);
if (ret < 0) {
dev_err(imx290->dev, "Could not set frame format\n");
return ret;
@@ -821,12 +847,6 @@ static int imx290_probe(struct i2c_client *client)
goto free_err;
}
- if (imx290->ep.link_frequencies[0] != IMX290_DEFAULT_LINK_FREQ) {
- dev_err(dev, "Unsupported link frequency\n");
- ret = -EINVAL;
- goto free_err;
- }
-
/* Only CSI2 is supported for now */
if (imx290->ep.bus_type != V4L2_MBUS_CSI2_DPHY) {
dev_err(dev, "Unsupported bus type, should be CSI2\n");
@@ -879,23 +899,38 @@ static int imx290_probe(struct i2c_client *client)
mutex_init(&imx290->lock);
+ /*
+ * Initialize the frame format. In particular, imx290->current_mode
+ * and imx290->bpp are set to defaults: imx290_calc_pixel_rate() call
+ * below relies on these fields.
+ */
+ imx290_entity_init_cfg(&imx290->sd, NULL);
+
v4l2_ctrl_handler_init(&imx290->ctrls, 3);
v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_GAIN, 0, 72, 1, 0);
- imx290->link_freq =
- v4l2_ctrl_new_int_menu(&imx290->ctrls,
- &imx290_ctrl_ops,
- V4L2_CID_LINK_FREQ,
- ARRAY_SIZE(imx290_link_freq) - 1,
- 0, imx290_link_freq);
+ if (imx290->nlanes == 4)
+ imx290->link_freq =
+ v4l2_ctrl_new_int_menu(&imx290->ctrls,
+ &imx290_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(imx290_link_freq_4lanes) - 1,
+ 0, imx290_link_freq_4lanes);
+ else
+ imx290->link_freq =
+ v4l2_ctrl_new_int_menu(&imx290->ctrls,
+ &imx290_ctrl_ops,
+ V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(imx290_link_freq_2lanes) - 1,
+ 0, imx290_link_freq_2lanes);
if (imx290->link_freq)
imx290->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
imx290->pixel_rate = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
V4L2_CID_PIXEL_RATE, 1,
INT_MAX, 1,
- imx290_modes[0].pixel_rate);
+ imx290_calc_pixel_rate(imx290));
imx290->sd.ctrl_handler = &imx290->ctrls;
@@ -919,9 +954,6 @@ static int imx290_probe(struct i2c_client *client)
goto free_ctrl;
}
- /* Initialize the frame format (this also sets imx290->current_mode) */
- imx290_entity_init_cfg(&imx290->sd, NULL);
-
ret = v4l2_async_register_subdev(&imx290->sd);
if (ret < 0) {
dev_err(dev, "Could not register v4l2 device\n");
--
2.17.1
^ permalink raw reply related
* [PATCH v3 02/10] media: i2c: imx290: fix the order of the args in SET_RUNTIME_PM_OPS()
From: Andrey Konovalov @ 2020-05-24 19:24 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
This macro is defined as SET_RUNTIME_PM_OPS(suspend_fn, resume_fn, idle_fn),
so imx290_power_off must be the 1st arg, and imx290_power_on the 2nd.
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/media/i2c/imx290.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 2d8c38ffe2f0..d0322f9a8856 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -648,7 +648,7 @@ static int imx290_power_off(struct device *dev)
}
static const struct dev_pm_ops imx290_pm_ops = {
- SET_RUNTIME_PM_OPS(imx290_power_on, imx290_power_off, NULL)
+ SET_RUNTIME_PM_OPS(imx290_power_off, imx290_power_on, NULL)
};
static const struct v4l2_subdev_video_ops imx290_video_ops = {
--
2.17.1
^ permalink raw reply related
* [PATCH v3 03/10] media: i2c: imx290: fix reset GPIO pin handling
From: Andrey Konovalov @ 2020-05-24 19:24 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
According to https://www.kernel.org/doc/Documentation/gpio/consumer.txt,
- all of the gpiod_set_value_xxx() functions operate with the *logical* value.
So in imx290_power_on() the reset signal should be cleared/de-asserted with
gpiod_set_value_cansleep(imx290->rst_gpio, 0), and in imx290_power_off() the
value of 1 must be used to apply/assert the reset to the sensor. In the device
tree the reset pin is described as GPIO_ACTIVE_LOW, and gpiod_set_value_xxx()
functions take this into account,
- when devm_gpiod_get_optional() is called with GPIOD_ASIS, the GPIO is not
initialized, and the direction must be set later; using a GPIO
without setting its direction first is illegal and will result in undefined
behavior. Fix this by using GPIOD_OUT_HIGH instead of GPIOD_ASIS (this asserts
the reset signal to the sensor initially).
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/media/i2c/imx290.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index d0322f9a8856..7b1de1f0c8b7 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -628,7 +628,7 @@ static int imx290_power_on(struct device *dev)
}
usleep_range(1, 2);
- gpiod_set_value_cansleep(imx290->rst_gpio, 1);
+ gpiod_set_value_cansleep(imx290->rst_gpio, 0);
usleep_range(30000, 31000);
return 0;
@@ -641,7 +641,7 @@ static int imx290_power_off(struct device *dev)
struct imx290 *imx290 = to_imx290(sd);
clk_disable_unprepare(imx290->xclk);
- gpiod_set_value_cansleep(imx290->rst_gpio, 0);
+ gpiod_set_value_cansleep(imx290->rst_gpio, 1);
regulator_bulk_disable(IMX290_NUM_SUPPLIES, imx290->supplies);
return 0;
@@ -757,7 +757,8 @@ static int imx290_probe(struct i2c_client *client)
goto free_err;
}
- imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS);
+ imx290->rst_gpio = devm_gpiod_get_optional(dev, "reset",
+ GPIOD_OUT_HIGH);
if (IS_ERR(imx290->rst_gpio)) {
dev_err(dev, "Cannot get reset gpio\n");
ret = PTR_ERR(imx290->rst_gpio);
--
2.17.1
^ permalink raw reply related
* [PATCH v3 01/10] media: i2c: imx290: set the format before VIDIOC_SUBDEV_G_FMT is called
From: Andrey Konovalov @ 2020-05-24 19:24 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
In-Reply-To: <20200524192505.20682-1-andrey.konovalov@linaro.org>
With the current driver 'media-ctl -p' issued right after the imx290 driver
is loaded prints:
pad0: Source
[fmt:unknown/0x0]
The format value of zero is due to the current_format field of the imx290
struct not being initialized yet.
As imx290_entity_init_cfg() calls imx290_set_fmt(), the current_mode field
is also initialized, so the line which set current_mode to a default value
in driver's probe() function is no longer needed.
Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
drivers/media/i2c/imx290.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index f7678e5a5d87..2d8c38ffe2f0 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -722,9 +722,6 @@ static int imx290_probe(struct i2c_client *client)
goto free_err;
}
- /* Set default mode to max resolution */
- imx290->current_mode = &imx290_modes[0];
-
/* get system clock (xclk) */
imx290->xclk = devm_clk_get(dev, "xclk");
if (IS_ERR(imx290->xclk)) {
@@ -809,6 +806,9 @@ static int imx290_probe(struct i2c_client *client)
goto free_ctrl;
}
+ /* Initialize the frame format (this also sets imx290->current_mode) */
+ imx290_entity_init_cfg(&imx290->sd, NULL);
+
ret = v4l2_async_register_subdev(&imx290->sd);
if (ret < 0) {
dev_err(dev, "Could not register v4l2 device\n");
--
2.17.1
^ permalink raw reply related
* [PATCH v3 00/10] Improvements to IMX290 CMOS driver
From: Andrey Konovalov @ 2020-05-24 19:24 UTC (permalink / raw)
To: mchehab, sakari.ailus, manivannan.sadhasivam
Cc: linux-media, linux-kernel, devicetree, linux-arm-kernel,
c.barrett, a.brela, peter.griffin, Andrey Konovalov
This patchset adds improvements to the existing media driver for IMX290
CMOS sensor from Sony. The major changes are adding 2 lane support,
configurable link frequency & pixel rate, test pattern generation, and
RAW12 mode support.
The link frequency & pixel rate combinations depend on various factors like
lane count, resolution and image format as per the datasheet.
Also fixes for the following issues in the existing driver are included:
* the current_format field in the struct imx290 can be used before
initialization,
* the reset signal to IMX290 isn't handled correctly,
* the bus_type field of v4l2_fwnode_endpoint structure passed as the
argument to v4l2_fwnode_endpoint_alloc_parse() function is not
initiaized.
Changes in v3:
* The review comments from Sakari are addressed
https://lkml.org/lkml/2019/12/19/705
As a part of those changes:
. null ptr checks are added to imx290_set_fmt() so that it can be called
early in the probe() function to set the default format, and to
initialize imx290->current_mode and imx290->bpp - these last two must be
set before imx290_calc_pixel_rate() is called when creating the controls
. setting imx290->bpp removed from imx290_write_current_format(). Now this
function only writes to the camera sensor registers. The call to
imx290_write_current_format() is moved from imx290_set_fmt() back to
imx290_start_streaming(): imx290_set_fmt() can be called when the sensor
is powered off, and writes to the sensor registers would fail.
. in imx290_set_ctrl() in the 12 bpp case the value the BLKLEVEL register
is restored to when the test pattern is disabled is made consistent with
imx290_12bit_settings[]
* The "IMX290 sensor driver fixes" patchset included
https://patchwork.kernel.org/cover/11407347/
* Added a patch to set the bus_type field of v4l2_fwnode_endpoint structure
before calling v4l2_fwnode_endpoint_alloc_parse()
Andrey Konovalov (4):
media: i2c: imx290: set the format before VIDIOC_SUBDEV_G_FMT is
called
media: i2c: imx290: fix the order of the args in SET_RUNTIME_PM_OPS()
media: i2c: imx290: fix reset GPIO pin handling
media: i2c: imx290: set bus_type before calling
v4l2_fwnode_endpoint_alloc_parse()
Manivannan Sadhasivam (6):
media: i2c: imx290: Add support for 2 data lanes
media: i2c: imx290: Add configurable link frequency and pixel rate
media: i2c: imx290: Add support for test pattern generation
media: i2c: imx290: Add RAW12 mode support
media: i2c: imx290: Add support to enumerate all frame sizes
media: i2c: imx290: Move the settle time delay out of loop
drivers/media/i2c/imx290.c | 358 ++++++++++++++++++++++++++++++-------
1 file changed, 297 insertions(+), 61 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH v2 3/4] mtd: rawnand: ingenic: Fix the RB gpio active-high property on qi, lb60
From: Miquel Raynal @ 2020-05-24 19:03 UTC (permalink / raw)
To: Boris Brezillon, Paul Cercueil, Harvey Hunt, Miquel Raynal,
linux-mtd
Cc: Mark Rutland, devicetree, Vignesh Raghavendra, Tudor Ambarus,
Richard Weinberger, Rob Herring
In-Reply-To: <20200519232454.374081-3-boris.brezillon@collabora.com>
On Tue, 2020-05-19 at 23:24:53 UTC, Boris Brezillon wrote:
> The rb-gpios semantics was undocumented and qi,lb60 (along with the
> ingenic driver) got it wrong. The active state encodes the NAND ready
> state, which is high level. Since there's no signal inverter on this
> board, it should be active-high. Let's fix that here for older DTs so
> we can re-use the generic nand_gpio_waitrdy() helper, and be consistent
> with what other drivers do.
>
> Suggested-by: Paul Cercueil <paul@crapouillou.net>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
^ permalink raw reply
* Re: [PATCH v2 4/4] mtd: rawnand: ingenic: Convert the driver to exec_op()
From: Miquel Raynal @ 2020-05-24 19:03 UTC (permalink / raw)
To: Boris Brezillon, Paul Cercueil, Harvey Hunt, Miquel Raynal,
linux-mtd
Cc: Mark Rutland, devicetree, Vignesh Raghavendra, Tudor Ambarus,
Richard Weinberger, Rob Herring
In-Reply-To: <20200519232454.374081-4-boris.brezillon@collabora.com>
On Tue, 2020-05-19 at 23:24:54 UTC, Boris Brezillon wrote:
> Let's convert the driver to exec_op() to have one less driver relying
> on the legacy interface.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Tested-by: Paul Cercueil <paul@crapouillou.net>
Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.
Miquel
^ permalink raw reply
* Re: [PATCH v5 0/6] Support DRM bridges on NVIDIA Tegra
From: Dmitry Osipenko @ 2020-05-24 18:41 UTC (permalink / raw)
To: Thierry Reding
Cc: Sam Ravnborg, Laurent Pinchart, Rob Herring, Frank Rowand,
dri-devel, linux-tegra, devicetree, linux-kernel
In-Reply-To: <20200418170703.1583-1-digetx@gmail.com>
18.04.2020 20:06, Dmitry Osipenko пишет:
> Hello,
>
> This series adds initial support for the DRM bridges to NVIDIA Tegra DRM
> driver. This is required by newer device-trees where we model the LVDS
> encoder bridge properly.
>
> Changelog:
>
> v5: - Added new patches that make drm_of_find_panel_or_bridge() more usable
> if graph isn't defined in a device-tree:
>
> of_graph: add of_graph_get_local_port()
> drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
>
> - Updated "Support DRM bridges" patch to use drm_of_find_panel_or_bridge()
> directly and added WARN_ON(output->panel || output->bridge) sanity-check.
>
> - Added new "Wrap directly-connected panel into DRM bridge" patch, as
> was suggested by Laurent Pinchart.
>
> v4: - Following review comments that were made by Laurent Pinchart to the v3,
> we now create and use the "bridge connector".
>
> v3: - Following recommendation from Sam Ravnborg, the new bridge attachment
> model is now being used, i.e. we ask bridge to *not* create a connector
> using the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
>
> - The bridge is now created only for the RGB (LVDS) output, and only
> when necessary. For now we don't need bridges for HDMI or DSI outputs.
>
> - I noticed that we're leaking OF node in the panel's error code path,
> this is fixed now by the new patch "Don't leak OF node on error".
>
> v2: - Added the new "rgb: Don't register connector if bridge is used"
> patch, which hides the unused connector provided by the Tegra DRM
> driver when bridge is used, since bridge provides its own connector
> to us.
>
> - Please notice that the first "Support DRM bridges" patch was previously
> sent out as a standalone v1 change.
>
> Dmitry Osipenko (6):
> of_graph: add of_graph_get_local_port()
> drm/of: Make drm_of_find_panel_or_bridge() to check graph's presence
> drm/tegra: output: Don't leak OF node on error
> drm/tegra: output: Support DRM bridges
> drm/tegra: output: rgb: Support LVDS encoder bridge
> drm/tegra: output: rgb: Wrap directly-connected panel into DRM bridge
>
> drivers/gpu/drm/drm_of.c | 13 +++++-
> drivers/gpu/drm/tegra/drm.h | 2 +
> drivers/gpu/drm/tegra/output.c | 21 +++++++--
> drivers/gpu/drm/tegra/rgb.c | 85 +++++++++++++++++++++-------------
> drivers/of/property.c | 32 +++++++++----
> include/linux/of_graph.h | 7 +++
> 6 files changed, 114 insertions(+), 46 deletions(-)
>
Hello Thierry,
Do you you have any objections to this series?
^ permalink raw reply
* Re: [PATCH v5 13/13] soc: mediatek: cmdq: add set event function
From: Matthias Brugger @ 2020-05-24 18:15 UTC (permalink / raw)
To: Dennis-YC Hsieh
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <1590341965.31286.27.camel@mtkswgap22>
On 24/05/2020 19:39, Dennis-YC Hsieh wrote:
> Hi Matthias,
>
> Thanks for your comment.
>
>
> On Sat, 2020-05-16 at 20:32 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> Add set event function in cmdq helper functions to set specific event.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
>>> include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
>>> include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
>>> 3 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index ec5637d43254..3294c9285994 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -327,6 +327,21 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
>>> }
>>> EXPORT_SYMBOL(cmdq_pkt_clear_event);
>>>
>>> +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
>>> +{
>>> + struct cmdq_instruction inst = { {0} };
>>> +
>>> + if (event >= CMDQ_MAX_EVENT)
>>> + return -EINVAL;
>>> +
>>> + inst.op = CMDQ_CODE_WFE;
>>> + inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
>>> + inst.event = event;
>>> +
>>> + return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_set_event);
>>> +
>>> int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>>> u16 offset, u32 value)
>>> {
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index 42d2a30e6a70..ba2d811183a9 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -17,6 +17,7 @@
>>> #define CMDQ_JUMP_PASS CMDQ_INST_SIZE
>>>
>>> #define CMDQ_WFE_UPDATE BIT(31)
>>> +#define CMDQ_WFE_UPDATE_VALUE BIT(16)
>>> #define CMDQ_WFE_WAIT BIT(15)
>>> #define CMDQ_WFE_WAIT_VALUE 0x1
>>>
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index d63749440697..ca70296ae120 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -168,6 +168,15 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
>>> */
>>> int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event);
>>>
>>> +/**
>>> + * cmdq_pkt_set_event() - append set event command to the CMDQ packet
>>> + * @pkt: the CMDQ packet
>>> + * @event: the desired event to be set
>>
>> Can we add the events and their code, so that later on, when a consumer calls
>> cmdq_pkt_set_event() we don't have any magic values that are hard to understand?
>
> Please see patch 02/13:
> http://lists.infradead.org/pipermail/linux-mediatek/2020-March/027801.html
>
> Definitions begin with CMDQ_EVENT_ is the event id to this function.
> Since the event id is different between platform, client must parse it
> from device tree. So no magic values require when call this function.
>
>
Got it, thanks for clarification.
Matthias
^ permalink raw reply
* Re: [PATCH v5 09/13] soc: mediatek: cmdq: add write_s value function
From: Matthias Brugger @ 2020-05-24 18:13 UTC (permalink / raw)
To: Dennis-YC Hsieh
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <1590341462.31286.19.camel@mtkswgap22>
On 24/05/2020 19:31, Dennis-YC Hsieh wrote:
> Hi Matthias,
>
> Thanks for your comment.
>
> On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> add write_s function in cmdq helper functions which
>>> writes a constant value to address with large dma
>>> access support.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
>>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++
>>> 2 files changed, 40 insertions(+)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 03c129230cd7..a9ebbabb7439 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> }
>>> EXPORT_SYMBOL(cmdq_pkt_write_s);
>>>
>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> + u16 addr_low, u32 value, u32 mask)
>>> +{
>>> + struct cmdq_instruction inst = { {0} };
>>> + int err;
>>> +
>>> + if (mask != U32_MAX) {
>>> + inst.op = CMDQ_CODE_MASK;
>>> + inst.mask = ~mask;
>>> + err = cmdq_pkt_append_command(pkt, inst);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + inst.op = CMDQ_CODE_WRITE_S_MASK;
>>> + } else {
>>> + inst.op = CMDQ_CODE_WRITE_S;
>>> + }
>>> +
>>> + inst.sop = high_addr_reg_idx;
>>
>> Writing u16 value in a 5 bit wide variable?
>
> We need only 5 bits in this case. I'll change high_addr_reg_idx
> parameter to u8.
>
Ok, please make sure to mask the value, so that it's explicit in the code that
we only use the lowest 5 bits of high_addr_reg_idx.
Regards,
Matthias
>>
>>> + inst.offset = addr_low;
>>> + inst.value = value;
>>> +
>>> + return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
>>> +
>>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>> {
>>> struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 01b4184af310..fec292aac83c 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
>>> int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> u16 addr_low, u16 src_reg_idx, u32 mask);
>>>
>>> +/**
>>> + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
>>> + * packet which write value to a physical address
>>> + * @pkt: the CMDQ packet
>>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa
>>
>> register
>
> will fix
>
>
> Regards,
> Dennis
>
>>
>>> + * @addr_low: low address of pa
>>> + * @value: the specified target value
>>> + * @mask: the specified target mask
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + */
>>> +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> + u16 addr_low, u32 value, u32 mask);
>>> +
>>> /**
>>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>> * @pkt: the CMDQ packet
>>>
>
^ permalink raw reply
* Re: [PATCH v5 07/13] soc: mediatek: cmdq: add write_s function
From: Matthias Brugger @ 2020-05-24 18:12 UTC (permalink / raw)
To: Dennis-YC Hsieh
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <1590341186.31286.16.camel@mtkswgap22>
On 24/05/2020 19:26, Dennis-YC Hsieh wrote:
> Hi Mattias,
>
> Thanks for your comment.
>
> On Sat, 2020-05-16 at 20:14 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> add write_s function in cmdq helper functions which
>>> writes value contains in internal register to address
>>> with large dma access support.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 34 +++++++++++++++++++++++-
>>> include/linux/mailbox/mtk-cmdq-mailbox.h | 2 ++
>>> include/linux/soc/mediatek/mtk-cmdq.h | 20 ++++++++++++++
>>> 3 files changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 33153d17c9d9..90f1ff2b4b00 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -18,6 +18,10 @@ struct cmdq_instruction {
>>> union {
>>> u32 value;
>>> u32 mask;
>>> + struct {
>>> + u16 arg_c;
>>> + u16 src_reg;
>>> + };
>>> };
>>> union {
>>> u16 offset;
>>> @@ -29,7 +33,7 @@ struct cmdq_instruction {
>>> struct {
>>> u8 sop:5;
>>> u8 arg_c_t:1;
>>> - u8 arg_b_t:1;
>>> + u8 src_t:1;
>>
>> fixing patch 6/13 please. seems the struct should be added in this patch.
>
> ok, will move to this patch.
>
>>
>>> u8 dst_t:1;
>>> };
>>> };
>>> @@ -222,6 +226,34 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>>> }
>>> EXPORT_SYMBOL(cmdq_pkt_write_mask);
>>>
>>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> + u16 addr_low, u16 src_reg_idx, u32 mask)
>>> +{
>>> + struct cmdq_instruction inst = { {0} };
>>> + int err;
>>> +
>>> + if (mask != U32_MAX) {
>>> + inst.op = CMDQ_CODE_MASK;
>>> + inst.mask = ~mask;
>>> + err = cmdq_pkt_append_command(pkt, inst);
>>> + if (err < 0)
>>> + return err;
>>> +
>>> + inst.mask = 0;
>>> + inst.op = CMDQ_CODE_WRITE_S_MASK;
>>> + } else {
>>> + inst.op = CMDQ_CODE_WRITE_S;
>>> + }
>>> +
>>> + inst.src_t = CMDQ_REG_TYPE;
>>
>> Not defined.
>> Please make sure that every patch compiles on it's own and does not add a
>> regression. This is very helpful if we have to bisect the kernel in the future.
>
> May I know which part not defined? The src_t defined on top of this
> patch and CMDQ_REG_TYPE defined in last patc (see 06/13).
correct, sorry for the noise.
>
>>
>>> + inst.sop = high_addr_reg_idx;
>>> + inst.offset = addr_low;
>>> + inst.src_reg = src_reg_idx;
>>> +
>>> + return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_write_s);
>>> +
>>> int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
>>> {
>>> struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index 121c3bb6d3de..8ef87e1bd03b 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -59,6 +59,8 @@ enum cmdq_code {
>>> CMDQ_CODE_JUMP = 0x10,
>>> CMDQ_CODE_WFE = 0x20,
>>> CMDQ_CODE_EOC = 0x40,
>>> + CMDQ_CODE_WRITE_S = 0x90,
>>> + CMDQ_CODE_WRITE_S_MASK = 0x91,
>>> CMDQ_CODE_LOGIC = 0xa0,
>>> };
>>>
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index 83340211e1d3..c72d826d8934 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -12,6 +12,8 @@
>>> #include <linux/timer.h>
>>>
>>> #define CMDQ_NO_TIMEOUT 0xffffffffu
>>> +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0)))
>>> +#define CMDQ_ADDR_LOW(addr) ((u16)(addr) | BIT(1))
>>>
>>> struct cmdq_pkt;
>>>
>>> @@ -102,6 +104,24 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value);
>>> int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
>>> u16 offset, u32 value, u32 mask);
>>>
>>> +/**
>>> + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
>>> + * @pkt: the CMDQ packet
>>> + * @high_addr_reg_idx: internal regisger ID which contains high address of pa
>>
>> s/regisger/register
>
> will fix
>
>>
>>> + * @addr_low: low address of pa
>>> + * @src_reg_idx: the CMDQ internal register ID which cache source value
>>> + * @mask: the specified target address mask, use U32_MAX if no need
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + *
>>> + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH()
>>> + * to get high addrees and call cmdq_pkt_assign() to assign value into internal
>>
>> s/addrees/address
>
> will fix
>
>>
>>> + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameterwhen
>>
>> s/parameterwhen/parameter when
>
> will fix
>
>>
>>> + * call to this function.
>>> + */
>>> +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
>>> + u16 addr_low, u16 src_reg_idx, u32 mask);
>>> +
>>
>> In general I wonder if we shouldn't provide two functions, one that writes a
>> mask and on for the else case.
>
> ok, I'll separate this function to cmdq_pkt_write_s and
> cmdq_pkt_write_s_mask. Let the client choose which case is more
> suitable.
Sound good, thanks.
>
>
>>
>> Regards,
>> Matthias
>>
>>> /**
>>> * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
>>> * @pkt: the CMDQ packet
>>>
>
^ permalink raw reply
* Re: [PATCH v5 06/13] soc: mediatek: cmdq: add assign function
From: Matthias Brugger @ 2020-05-24 18:09 UTC (permalink / raw)
To: Dennis-YC Hsieh
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <1590339695.31286.4.camel@mtkswgap22>
On 24/05/2020 19:01, Dennis-YC Hsieh wrote:
> Hi Matthias,
>
> Thanks for your comment.
>
> On Sat, 2020-05-16 at 19:59 +0200, Matthias Brugger wrote:
>>
>> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
>>> Add assign function in cmdq helper which assign constant value into
>>> internal register by index.
>>>
>>> Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
>>> Reviewed-by: CK Hu <ck.hu@mediatek.com>
>>> ---
>>> drivers/soc/mediatek/mtk-cmdq-helper.c | 24 +++++++++++++++++++++++-
>>> include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
>>> include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++
>>> 3 files changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> index 98f23ba3ba47..33153d17c9d9 100644
>>> --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
>>> @@ -12,6 +12,7 @@
>>> #define CMDQ_WRITE_ENABLE_MASK BIT(0)
>>> #define CMDQ_POLL_ENABLE_MASK BIT(0)
>>> #define CMDQ_EOC_IRQ_EN BIT(0)
>>> +#define CMDQ_REG_TYPE 1
>>>
>>> struct cmdq_instruction {
>>> union {
>>> @@ -21,8 +22,17 @@ struct cmdq_instruction {
>>> union {
>>> u16 offset;
>>> u16 event;
>>> + u16 reg_dst;
>>> + };
>>> + union {
>>> + u8 subsys;
>>> + struct {
>>> + u8 sop:5;
>>> + u8 arg_c_t:1;
>>> + u8 arg_b_t:1;
>>> + u8 dst_t:1;
>>> + };
>>
>> This union seems without context in this patch. Please drop.
>>
>
> The dst_t use in cmdq_pkt_assign function so how about merge other
Ah didn't realize this. Then I think it's OK like it is.
Regards,
Matthias
> variables to reserved and leave dst_t ?
>
> struct {
> u8 reserved_t:7;
> u8 dst_t:1;
> };
>
>
> Regards,
> Dennis
>
>
>> Regards,
>> Matthias
>>
>>> };
>>> - u8 subsys;
>>> u8 op;
>>> };
>>>
>>> @@ -277,6 +287,18 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>>> }
>>> EXPORT_SYMBOL(cmdq_pkt_poll_mask);
>>>
>>> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
>>> +{
>>> + struct cmdq_instruction inst = { {0} };
>>> +
>>> + inst.op = CMDQ_CODE_LOGIC;
>>> + inst.dst_t = CMDQ_REG_TYPE;
>>> + inst.reg_dst = reg_idx;
>>> + inst.value = value;
>>> + return cmdq_pkt_append_command(pkt, inst);
>>> +}
>>> +EXPORT_SYMBOL(cmdq_pkt_assign);
>>> +
>>> static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
>>> {
>>> struct cmdq_instruction inst = { {0} };
>>> diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> index dfe5b2eb85cc..121c3bb6d3de 100644
>>> --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
>>> @@ -59,6 +59,7 @@ enum cmdq_code {
>>> CMDQ_CODE_JUMP = 0x10,
>>> CMDQ_CODE_WFE = 0x20,
>>> CMDQ_CODE_EOC = 0x40,
>>> + CMDQ_CODE_LOGIC = 0xa0,
>>> };
>>>
>>> enum cmdq_cb_status {
>>> diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
>>> index a74c1d5acdf3..83340211e1d3 100644
>>> --- a/include/linux/soc/mediatek/mtk-cmdq.h
>>> +++ b/include/linux/soc/mediatek/mtk-cmdq.h
>>> @@ -152,6 +152,20 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
>>> */
>>> int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
>>> u16 offset, u32 value, u32 mask);
>>> +
>>> +/**
>>> + * cmdq_pkt_assign() - Append logic assign command to the CMDQ packet, ask GCE
>>> + * to execute an instruction that set a constant value into
>>> + * internal register and use as value, mask or address in
>>> + * read/write instruction.
>>> + * @pkt: the CMDQ packet
>>> + * @reg_idx: the CMDQ internal register ID
>>> + * @value: the specified value
>>> + *
>>> + * Return: 0 for success; else the error code is returned
>>> + */
>>> +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
>>> +
>>> /**
>>> * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
>>> * packet and call back at the end of done packet
>>>
>
^ permalink raw reply
* Re: [PATCH v5 13/13] soc: mediatek: cmdq: add set event function
From: Dennis-YC Hsieh @ 2020-05-24 17:39 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <d00dd874-edda-701a-2acc-540ed19d1300@gmail.com>
Hi Matthias,
Thanks for your comment.
On Sat, 2020-05-16 at 20:32 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add set event function in cmdq helper functions to set specific event.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 15 +++++++++++++++
> > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
> > include/linux/soc/mediatek/mtk-cmdq.h | 9 +++++++++
> > 3 files changed, 25 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index ec5637d43254..3294c9285994 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -327,6 +327,21 @@ int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event)
> > }
> > EXPORT_SYMBOL(cmdq_pkt_clear_event);
> >
> > +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > +
> > + if (event >= CMDQ_MAX_EVENT)
> > + return -EINVAL;
> > +
> > + inst.op = CMDQ_CODE_WFE;
> > + inst.value = CMDQ_WFE_UPDATE | CMDQ_WFE_UPDATE_VALUE;
> > + inst.event = event;
> > +
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_set_event);
> > +
> > int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value)
> > {
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 42d2a30e6a70..ba2d811183a9 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -17,6 +17,7 @@
> > #define CMDQ_JUMP_PASS CMDQ_INST_SIZE
> >
> > #define CMDQ_WFE_UPDATE BIT(31)
> > +#define CMDQ_WFE_UPDATE_VALUE BIT(16)
> > #define CMDQ_WFE_WAIT BIT(15)
> > #define CMDQ_WFE_WAIT_VALUE 0x1
> >
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index d63749440697..ca70296ae120 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -168,6 +168,15 @@ int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> > */
> > int cmdq_pkt_clear_event(struct cmdq_pkt *pkt, u16 event);
> >
> > +/**
> > + * cmdq_pkt_set_event() - append set event command to the CMDQ packet
> > + * @pkt: the CMDQ packet
> > + * @event: the desired event to be set
>
> Can we add the events and their code, so that later on, when a consumer calls
> cmdq_pkt_set_event() we don't have any magic values that are hard to understand?
Please see patch 02/13:
http://lists.infradead.org/pipermail/linux-mediatek/2020-March/027801.html
Definitions begin with CMDQ_EVENT_ is the event id to this function.
Since the event id is different between platform, client must parse it
from device tree. So no magic values require when call this function.
Regard,
Dennis
>
> Regards,
> Matthias
>
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_set_event(struct cmdq_pkt *pkt, u16 event);
> > +
> > /**
> > * cmdq_pkt_poll() - Append polling command to the CMDQ packet, ask GCE to
> > * execute an instruction that wait for a specified
> >
^ permalink raw reply
* Re: [PATCH v5 12/13] soc: mediatek: cmdq: add clear option in cmdq_pkt_wfe api
From: Dennis-YC Hsieh @ 2020-05-24 17:32 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <6029e0eb-70fa-825f-7fd4-f5127384bd8d@gmail.com>
Hi Matthias,
Thanks for your comment.
On Sat, 2020-05-16 at 20:30 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add clear parameter to let client decide if
> > event should be clear to 0 after GCE receive it.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 2 +-
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 5 +++--
> > include/linux/mailbox/mtk-cmdq-mailbox.h | 3 +--
> > include/linux/soc/mediatek/mtk-cmdq.h | 5 +++--
> > 4 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 7daaabc26eb1..a065b3a412cf 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -488,7 +488,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> > if (mtk_crtc->cmdq_client) {
> > cmdq_handle = cmdq_pkt_create(mtk_crtc->cmdq_client, PAGE_SIZE);
> > cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> > - cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> > + cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event, false);
> > mtk_crtc_ddp_config(crtc, cmdq_handle);
> > cmdq_pkt_finalize(cmdq_handle);
> > cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
>
> This should be an independent patch
ok, I'll separate this part.
Regards,
Dennis
>
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index bb5be20fc70a..ec5637d43254 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -296,15 +296,16 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> >
> > -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear)
> > {
> > struct cmdq_instruction inst = { {0} };
> > + u32 clear_option = clear ? CMDQ_WFE_UPDATE : 0;
> >
> > if (event >= CMDQ_MAX_EVENT)
> > return -EINVAL;
> >
> > inst.op = CMDQ_CODE_WFE;
> > - inst.value = CMDQ_WFE_OPTION;
> > + inst.value = CMDQ_WFE_OPTION | clear_option;
> > inst.event = event;
> >
> > return cmdq_pkt_append_command(pkt, inst);
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 3f6bc0dfd5da..42d2a30e6a70 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -27,8 +27,7 @@
> > * bit 16-27: update value
> > * bit 31: 1 - update, 0 - no update
> > */
> > -#define CMDQ_WFE_OPTION (CMDQ_WFE_UPDATE | CMDQ_WFE_WAIT | \
> > - CMDQ_WFE_WAIT_VALUE)
> > +#define CMDQ_WFE_OPTION (CMDQ_WFE_WAIT | CMDQ_WFE_WAIT_VALUE)
> >
> > /** cmdq event maximum */
> > #define CMDQ_MAX_EVENT 0x3ff
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 1a6c56f3bec1..d63749440697 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -152,11 +152,12 @@ int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > /**
> > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> > * @pkt: the CMDQ packet
> > - * @event: the desired event type to "wait and CLEAR"
> > + * @event: the desired event type to wait
> > + * @clear: clear event or not after event arrive
> > *
> > * Return: 0 for success; else the error code is returned
> > */
> > -int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event);
> > +int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event, bool clear);
> >
> > /**
> > * cmdq_pkt_clear_event() - append clear event command to the CMDQ packet
> >
^ permalink raw reply
* Re: [PATCH v5 10/13] soc: mediatek: cmdq: export finalize function
From: Dennis-YC Hsieh @ 2020-05-24 17:32 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <5d6b61b2-23c9-647f-fa22-73e779010bd8@gmail.com>
Hi Matthias,
Thanks for your comment.
On Sat, 2020-05-16 at 20:22 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Export finalize function to client which helps append eoc and jump
> > command to pkt. Let client decide call finalize or not.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 +
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 7 ++-----
> > include/linux/soc/mediatek/mtk-cmdq.h | 8 ++++++++
> > 3 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > index 0dfcd1787e65..7daaabc26eb1 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> > @@ -490,6 +490,7 @@ static void mtk_drm_crtc_hw_config(struct mtk_drm_crtc *mtk_crtc)
> > cmdq_pkt_clear_event(cmdq_handle, mtk_crtc->cmdq_event);
> > cmdq_pkt_wfe(cmdq_handle, mtk_crtc->cmdq_event);
> > mtk_crtc_ddp_config(crtc, cmdq_handle);
> > + cmdq_pkt_finalize(cmdq_handle);
> > cmdq_pkt_flush_async(cmdq_handle, ddp_cmdq_cb, cmdq_handle);
> > }
> > #endif
>
> This should be a independent patch.
> Other then that patch looks good.
ok, I'll separate this part.
Regards,
Dennis
>
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index a9ebbabb7439..59bc1164b411 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -372,7 +372,7 @@ int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > }
> > EXPORT_SYMBOL(cmdq_pkt_assign);
> >
> > -static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > +int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > {
> > struct cmdq_instruction inst = { {0} };
> > int err;
> > @@ -392,6 +392,7 @@ static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> >
> > return err;
> > }
> > +EXPORT_SYMBOL(cmdq_pkt_finalize);
> >
> > static void cmdq_pkt_flush_async_cb(struct cmdq_cb_data data)
> > {
> > @@ -426,10 +427,6 @@ int cmdq_pkt_flush_async(struct cmdq_pkt *pkt, cmdq_async_flush_cb cb,
> > unsigned long flags = 0;
> > struct cmdq_client *client = (struct cmdq_client *)pkt->cl;
> >
> > - err = cmdq_pkt_finalize(pkt);
> > - if (err < 0)
> > - return err;
> > -
> > pkt->cb.cb = cb;
> > pkt->cb.data = data;
> > pkt->async_cb.cb = cmdq_pkt_flush_async_cb;
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index fec292aac83c..99e77155f967 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -213,6 +213,14 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > */
> > int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> >
> > +/**
> > + * cmdq_pkt_finalize() - Append EOC and jump command to pkt.
> > + * @pkt: the CMDQ packet
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_finalize(struct cmdq_pkt *pkt);
> > +
> > /**
> > * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> > * packet and call back at the end of done packet
> >
^ permalink raw reply
* Re: [PATCH v5 09/13] soc: mediatek: cmdq: add write_s value function
From: Dennis-YC Hsieh @ 2020-05-24 17:31 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <f9fd9ea8-f706-ed4a-4c83-c53ad092035c@gmail.com>
Hi Matthias,
Thanks for your comment.
On Sat, 2020-05-16 at 20:20 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > add write_s function in cmdq helper functions which
> > writes a constant value to address with large dma
> > access support.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 26 ++++++++++++++++++++++++++
> > include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++
> > 2 files changed, 40 insertions(+)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 03c129230cd7..a9ebbabb7439 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -269,6 +269,32 @@ int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_write_s);
> >
> > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > + u16 addr_low, u32 value, u32 mask)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > + int err;
> > +
> > + if (mask != U32_MAX) {
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.mask = ~mask;
> > + err = cmdq_pkt_append_command(pkt, inst);
> > + if (err < 0)
> > + return err;
> > +
> > + inst.op = CMDQ_CODE_WRITE_S_MASK;
> > + } else {
> > + inst.op = CMDQ_CODE_WRITE_S;
> > + }
> > +
> > + inst.sop = high_addr_reg_idx;
>
> Writing u16 value in a 5 bit wide variable?
We need only 5 bits in this case. I'll change high_addr_reg_idx
parameter to u8.
>
> > + inst.offset = addr_low;
> > + inst.value = value;
> > +
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_write_s_value);
> > +
> > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> > {
> > struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 01b4184af310..fec292aac83c 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -135,6 +135,20 @@ int cmdq_pkt_read_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx, u16 addr_low,
> > int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > u16 addr_low, u16 src_reg_idx, u32 mask);
> >
> > +/**
> > + * cmdq_pkt_write_s_value() - append write_s command with mask to the CMDQ
> > + * packet which write value to a physical address
> > + * @pkt: the CMDQ packet
> > + * @high_addr_reg_idx: internal regisger ID which contains high address of pa
>
> register
will fix
Regards,
Dennis
>
> > + * @addr_low: low address of pa
> > + * @value: the specified target value
> > + * @mask: the specified target mask
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_write_s_value(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > + u16 addr_low, u32 value, u32 mask);
> > +
> > /**
> > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> > * @pkt: the CMDQ packet
> >
^ permalink raw reply
* Re: [PATCH v5 07/13] soc: mediatek: cmdq: add write_s function
From: Dennis-YC Hsieh @ 2020-05-24 17:26 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <58575c2c-0b5a-55c0-f3b0-082bfd4b6144@gmail.com>
Hi Mattias,
Thanks for your comment.
On Sat, 2020-05-16 at 20:14 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > add write_s function in cmdq helper functions which
> > writes value contains in internal register to address
> > with large dma access support.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 34 +++++++++++++++++++++++-
> > include/linux/mailbox/mtk-cmdq-mailbox.h | 2 ++
> > include/linux/soc/mediatek/mtk-cmdq.h | 20 ++++++++++++++
> > 3 files changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 33153d17c9d9..90f1ff2b4b00 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -18,6 +18,10 @@ struct cmdq_instruction {
> > union {
> > u32 value;
> > u32 mask;
> > + struct {
> > + u16 arg_c;
> > + u16 src_reg;
> > + };
> > };
> > union {
> > u16 offset;
> > @@ -29,7 +33,7 @@ struct cmdq_instruction {
> > struct {
> > u8 sop:5;
> > u8 arg_c_t:1;
> > - u8 arg_b_t:1;
> > + u8 src_t:1;
>
> fixing patch 6/13 please. seems the struct should be added in this patch.
ok, will move to this patch.
>
> > u8 dst_t:1;
> > };
> > };
> > @@ -222,6 +226,34 @@ int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_write_mask);
> >
> > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > + u16 addr_low, u16 src_reg_idx, u32 mask)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > + int err;
> > +
> > + if (mask != U32_MAX) {
> > + inst.op = CMDQ_CODE_MASK;
> > + inst.mask = ~mask;
> > + err = cmdq_pkt_append_command(pkt, inst);
> > + if (err < 0)
> > + return err;
> > +
> > + inst.mask = 0;
> > + inst.op = CMDQ_CODE_WRITE_S_MASK;
> > + } else {
> > + inst.op = CMDQ_CODE_WRITE_S;
> > + }
> > +
> > + inst.src_t = CMDQ_REG_TYPE;
>
> Not defined.
> Please make sure that every patch compiles on it's own and does not add a
> regression. This is very helpful if we have to bisect the kernel in the future.
May I know which part not defined? The src_t defined on top of this
patch and CMDQ_REG_TYPE defined in last patc (see 06/13).
>
> > + inst.sop = high_addr_reg_idx;
> > + inst.offset = addr_low;
> > + inst.src_reg = src_reg_idx;
> > +
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_write_s);
> > +
> > int cmdq_pkt_wfe(struct cmdq_pkt *pkt, u16 event)
> > {
> > struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index 121c3bb6d3de..8ef87e1bd03b 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -59,6 +59,8 @@ enum cmdq_code {
> > CMDQ_CODE_JUMP = 0x10,
> > CMDQ_CODE_WFE = 0x20,
> > CMDQ_CODE_EOC = 0x40,
> > + CMDQ_CODE_WRITE_S = 0x90,
> > + CMDQ_CODE_WRITE_S_MASK = 0x91,
> > CMDQ_CODE_LOGIC = 0xa0,
> > };
> >
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index 83340211e1d3..c72d826d8934 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -12,6 +12,8 @@
> > #include <linux/timer.h>
> >
> > #define CMDQ_NO_TIMEOUT 0xffffffffu
> > +#define CMDQ_ADDR_HIGH(addr) ((u32)(((addr) >> 16) & GENMASK(31, 0)))
> > +#define CMDQ_ADDR_LOW(addr) ((u16)(addr) | BIT(1))
> >
> > struct cmdq_pkt;
> >
> > @@ -102,6 +104,24 @@ int cmdq_pkt_write(struct cmdq_pkt *pkt, u8 subsys, u16 offset, u32 value);
> > int cmdq_pkt_write_mask(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value, u32 mask);
> >
> > +/**
> > + * cmdq_pkt_write_s() - append write_s command to the CMDQ packet
> > + * @pkt: the CMDQ packet
> > + * @high_addr_reg_idx: internal regisger ID which contains high address of pa
>
> s/regisger/register
will fix
>
> > + * @addr_low: low address of pa
> > + * @src_reg_idx: the CMDQ internal register ID which cache source value
> > + * @mask: the specified target address mask, use U32_MAX if no need
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + *
> > + * Support write value to physical address without subsys. Use CMDQ_ADDR_HIGH()
> > + * to get high addrees and call cmdq_pkt_assign() to assign value into internal
>
> s/addrees/address
will fix
>
> > + * reg. Also use CMDQ_ADDR_LOW() to get low address for addr_low parameterwhen
>
> s/parameterwhen/parameter when
will fix
>
> > + * call to this function.
> > + */
> > +int cmdq_pkt_write_s(struct cmdq_pkt *pkt, u16 high_addr_reg_idx,
> > + u16 addr_low, u16 src_reg_idx, u32 mask);
> > +
>
> In general I wonder if we shouldn't provide two functions, one that writes a
> mask and on for the else case.
ok, I'll separate this function to cmdq_pkt_write_s and
cmdq_pkt_write_s_mask. Let the client choose which case is more
suitable.
>
> Regards,
> Matthias
>
> > /**
> > * cmdq_pkt_wfe() - append wait for event command to the CMDQ packet
> > * @pkt: the CMDQ packet
> >
^ permalink raw reply
* Re: [PATCH v5 06/13] soc: mediatek: cmdq: add assign function
From: Dennis-YC Hsieh @ 2020-05-24 17:01 UTC (permalink / raw)
To: Matthias Brugger
Cc: Rob Herring, Mark Rutland, Jassi Brar, Philipp Zabel,
David Airlie, Daniel Vetter, linux-kernel, linux-mediatek,
devicetree, wsd_upstream, dri-devel, Bibby Hsieh, CK Hu,
Houlong Wei, linux-arm-kernel, HS Liao
In-Reply-To: <c671ea8a-07fa-a050-4679-c7aa15d19a9d@gmail.com>
Hi Matthias,
Thanks for your comment.
On Sat, 2020-05-16 at 19:59 +0200, Matthias Brugger wrote:
>
> On 08/03/2020 11:52, Dennis YC Hsieh wrote:
> > Add assign function in cmdq helper which assign constant value into
> > internal register by index.
> >
> > Signed-off-by: Dennis YC Hsieh <dennis-yc.hsieh@mediatek.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> > drivers/soc/mediatek/mtk-cmdq-helper.c | 24 +++++++++++++++++++++++-
> > include/linux/mailbox/mtk-cmdq-mailbox.h | 1 +
> > include/linux/soc/mediatek/mtk-cmdq.h | 14 ++++++++++++++
> > 3 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/soc/mediatek/mtk-cmdq-helper.c b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > index 98f23ba3ba47..33153d17c9d9 100644
> > --- a/drivers/soc/mediatek/mtk-cmdq-helper.c
> > +++ b/drivers/soc/mediatek/mtk-cmdq-helper.c
> > @@ -12,6 +12,7 @@
> > #define CMDQ_WRITE_ENABLE_MASK BIT(0)
> > #define CMDQ_POLL_ENABLE_MASK BIT(0)
> > #define CMDQ_EOC_IRQ_EN BIT(0)
> > +#define CMDQ_REG_TYPE 1
> >
> > struct cmdq_instruction {
> > union {
> > @@ -21,8 +22,17 @@ struct cmdq_instruction {
> > union {
> > u16 offset;
> > u16 event;
> > + u16 reg_dst;
> > + };
> > + union {
> > + u8 subsys;
> > + struct {
> > + u8 sop:5;
> > + u8 arg_c_t:1;
> > + u8 arg_b_t:1;
> > + u8 dst_t:1;
> > + };
>
> This union seems without context in this patch. Please drop.
>
The dst_t use in cmdq_pkt_assign function so how about merge other
variables to reserved and leave dst_t ?
struct {
u8 reserved_t:7;
u8 dst_t:1;
};
Regards,
Dennis
> Regards,
> Matthias
>
> > };
> > - u8 subsys;
> > u8 op;
> > };
> >
> > @@ -277,6 +287,18 @@ int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > }
> > EXPORT_SYMBOL(cmdq_pkt_poll_mask);
> >
> > +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value)
> > +{
> > + struct cmdq_instruction inst = { {0} };
> > +
> > + inst.op = CMDQ_CODE_LOGIC;
> > + inst.dst_t = CMDQ_REG_TYPE;
> > + inst.reg_dst = reg_idx;
> > + inst.value = value;
> > + return cmdq_pkt_append_command(pkt, inst);
> > +}
> > +EXPORT_SYMBOL(cmdq_pkt_assign);
> > +
> > static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > {
> > struct cmdq_instruction inst = { {0} };
> > diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > index dfe5b2eb85cc..121c3bb6d3de 100644
> > --- a/include/linux/mailbox/mtk-cmdq-mailbox.h
> > +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h
> > @@ -59,6 +59,7 @@ enum cmdq_code {
> > CMDQ_CODE_JUMP = 0x10,
> > CMDQ_CODE_WFE = 0x20,
> > CMDQ_CODE_EOC = 0x40,
> > + CMDQ_CODE_LOGIC = 0xa0,
> > };
> >
> > enum cmdq_cb_status {
> > diff --git a/include/linux/soc/mediatek/mtk-cmdq.h b/include/linux/soc/mediatek/mtk-cmdq.h
> > index a74c1d5acdf3..83340211e1d3 100644
> > --- a/include/linux/soc/mediatek/mtk-cmdq.h
> > +++ b/include/linux/soc/mediatek/mtk-cmdq.h
> > @@ -152,6 +152,20 @@ int cmdq_pkt_poll(struct cmdq_pkt *pkt, u8 subsys,
> > */
> > int cmdq_pkt_poll_mask(struct cmdq_pkt *pkt, u8 subsys,
> > u16 offset, u32 value, u32 mask);
> > +
> > +/**
> > + * cmdq_pkt_assign() - Append logic assign command to the CMDQ packet, ask GCE
> > + * to execute an instruction that set a constant value into
> > + * internal register and use as value, mask or address in
> > + * read/write instruction.
> > + * @pkt: the CMDQ packet
> > + * @reg_idx: the CMDQ internal register ID
> > + * @value: the specified value
> > + *
> > + * Return: 0 for success; else the error code is returned
> > + */
> > +int cmdq_pkt_assign(struct cmdq_pkt *pkt, u16 reg_idx, u32 value);
> > +
> > /**
> > * cmdq_pkt_flush_async() - trigger CMDQ to asynchronously execute the CMDQ
> > * packet and call back at the end of done packet
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox