Hi Jacopo,
On 15/06/2020 09:47, Jacopo Mondi wrote:
> Hi Kieran,
>
> On Mon, Jun 15, 2020 at 09:40:42AM +0100, Kieran Bingham wrote:
>> Hi Jacopo,
>>
>> On 15/06/2020 09:43, Jacopo Mondi wrote:
>>> Hi Sakari,
>>>
>>> On Mon, Jun 15, 2020 at 11:34:06AM +0300, Sakari Ailus wrote:
>>>> Hi Jacopo,
>>>>
>>>> On Thu, Jun 11, 2020 at 06:16:51PM +0200, Jacopo Mondi wrote:
>>>>> Use the newly introduced get_mbus_config() subdevice pad operation to
>>>>> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
>>>>> the number of active data lanes accordingly.
>>>>>
>>>>> In order to be able to call the remote subdevice operation cache the
>>>>> index of the remote pad connected to the single CSI-2 input port.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
>>>>> ---
>>>>> drivers/media/platform/rcar-vin/rcar-csi2.c | 61 ++++++++++++++++++++-
>>>>> 1 file changed, 58 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> index 151e6a90c5fb..11769f004fd8 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
>>>>> @@ -363,6 +363,7 @@ struct rcar_csi2 {
>>>>> struct v4l2_async_notifier notifier;
>>>>> struct v4l2_async_subdev asd;
>>>>> struct v4l2_subdev *remote;
>>>>> + unsigned int remote_pad;
>>>>>
>>>>> struct v4l2_mbus_framefmt mf;
>>>>>
>>>>> @@ -371,6 +372,7 @@ struct rcar_csi2 {
>>>>>
>>>>> unsigned short lanes;
>>>>> unsigned char lane_swap[4];
>>>>> + unsigned short active_lanes;
>>>>
>>>> Do you need this? I.e. should you not always request this from the
>>>> transmitter device?
>>>
>>> It's actually the other way around. The receiver queries the
>>> transmitter to know how many data lanes it intends to use and adjusts
>>> its configuration to accommodate it.
>>>
>>>>
>>>>> };
>>>>>
>>>>> static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
>>>>> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
>>>>>
>>>>> /* Wait for the clock and data lanes to enter LP-11 state. */
>>>>> for (timeout = 0; timeout <= 20; timeout++) {
>>>>> - const u32 lane_mask = (1 << priv->lanes) - 1;
>>>>> + const u32 lane_mask = (1 << priv->active_lanes) - 1;
>>>>>
>>>>> if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) &&
>>>>> (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
>>>>> @@ -471,11 +473,57 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>>>>> * bps = link_freq * 2
>>>>> */
>>>>> mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
>>>>> - do_div(mbps, priv->lanes * 1000000);
>>>>> + do_div(mbps, priv->active_lanes * 1000000);
>>>>>
>>>>> return mbps;
>>>>> }
>>>>>
>>>>> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
>>>>> +{
>>>>> + struct v4l2_mbus_config mbus_config = { 0 };
>>>>> + unsigned int num_lanes = (-1U);
>>>>> + int ret;
>>>>> +
>>>>> + priv->active_lanes = priv->lanes;
>>>>> + ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
>>>>> + priv->remote_pad, &mbus_config);
>>>>> + if (ret == -ENOIOCTLCMD) {
>>>>> + dev_dbg(priv->dev, "No remote mbus configuration available\n");
>>>>> + return 0;
>>>>> + }
>>>>> +
>>>>> + if (ret) {
>>>>> + dev_err(priv->dev, "Failed to get remote mbus configuration\n");
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
>>>>> + dev_err(priv->dev, "Unsupported media bus type %u\n",
>>>>> + mbus_config.type);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + if (mbus_config.flags & V4L2_MBUS_CSI2_1_LANE)
>>>>> + num_lanes = 1;
>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_2_LANE)
>>>>> + num_lanes = 2;
>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_3_LANE)
>>>>> + num_lanes = 3;
>>>>> + else if (mbus_config.flags & V4L2_MBUS_CSI2_4_LANE)
>>>>> + num_lanes = 4;
>>>>
>>>> This is the downside of using flags... Anyway, I guess this is certainly
>>>> fine now.
>>>>
>>>
>>> Let's change this on top, if we like to (and I would like to :)
>>
>> That answers my question then ;-)
>>
>
> There's been a discussion on one of the first version of the series
> where I tried replacing flags and introduced a union of structures
> with fields, most likely similar to what you suggested.
>
> Based on the received comments I decided to use the existing
> V4L2_MBUS_* flags to ease the replacement of the video ops
> g|s_mbus_config() to minimize the changes. We could then on top
> replace flags.
That sound fine to me.
When I first saw this patch, I thought that this series was introducing
the V4L2_MBUS_CSI2_4_LANE flag types, but I see they are pre-existing.
I think it's perfectly reasonable to use the existing infrastructure for
this series, and adapt after.
--
Kieran
>
> Thanks
> j
>
>> --
>> Kieran
>>
>>
>>>
>>> Thanks
>>> j
>>>
>>>>> +
>>>>> + if (num_lanes > priv->lanes) {
>>>>> + dev_err(priv->dev,
>>>>> + "Unsupported mbus config: too many data lanes %u\n",
>>>>> + num_lanes);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + priv->active_lanes = num_lanes;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>> {
>>>>> const struct rcar_csi2_format *format;
>>>>> @@ -490,6 +538,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>> /* Code is validated in set_fmt. */
>>>>> format = rcsi2_code_to_fmt(priv->mf.code);
>>>>>
>>>>> + /* Get the remote mbus config to get the number of enabled lanes. */
>>>>> + ret = rcsi2_config_active_lanes(priv);
>>>>> + if (ret)
>>>>> + return ret;
>>>>> +
>>>>> /*
>>>>> * Enable all supported CSI-2 channels with virtual channel and
>>>>> * data type matching.
>>>>> @@ -522,7 +575,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>>>>> }
>>>>>
>>>>> phycnt = PHYCNT_ENABLECLK;
>>>>> - phycnt |= (1 << priv->lanes) - 1;
>>>>> + phycnt |= (1 << priv->active_lanes) - 1;
>>>>>
>>>>> mbps = rcsi2_calc_mbps(priv, format->bpp);
>>>>> if (mbps < 0)
>>>>> @@ -748,6 +801,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
>>>>> }
>>>>>
>>>>> priv->remote = subdev;
>>>>> + priv->remote_pad = pad;
>>>>>
>>>>> dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
>>>>>
>>>>> @@ -793,6 +847,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>>>>> priv->lanes);
>>>>> return -EINVAL;
>>>>> }
>>>>> + priv->active_lanes = priv->lanes;
>>>>>
>>>>> for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>>>>> priv->lane_swap[i] = i < priv->lanes ?
>>>>
>>>> --
>>>> Kind regards,
>>>>
>>>> Sakari Ailus
>>
>> --
>> Regards
>> --
>> Kieran
--
Regards
--
Kieran
