| Message ID | 20190316154801.20460-6-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: Implement negotiation of CSI-2 data lanes | expand |
Hi Jacopo, Thanks for your patch. On 2019-03-16 16:48:01 +0100, Jacopo Mondi wrote: > Use the D-PHY configuration reported by the remote subdevice in its > frame descriptor to configure the interface. > > Store the number of lanes reported through the 'data-lanes' DT property > as the number of phyisically available lanes, which might not correspond > to the number of lanes actually in use. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++-------- > 1 file changed, 43 insertions(+), 28 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > index 6c46bcc0ee83..70b9a8165a6e 100644 > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > @@ -375,8 +375,8 @@ struct rcar_csi2 { > struct mutex lock; > int stream_count; > > - unsigned short lanes; > - unsigned char lane_swap[4]; > + unsigned short available_data_lanes; > + unsigned short num_data_lanes; I don't like this, I'm sorry :-( I think you should keep lanes and lane_swap and most code touching them intact. And drop num_data_lanes as it's only used when starting and only valid for one 'session' and should not be cached in the data structure unnecessary. Furthermore I think involving lane swapping in the runtime configurable parameters is a bad idea. Or do you see a scenario where lanes could be swapped in runtime? In my view DT should describe which physical lanes are connected and how. And the runtime configuration part should only deal with how many lanes are used for the particular capture session. > }; > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv, > if (ret) > return -ENODEV; > > - if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) { > dev_err(priv->dev, "Frame desc do not describe CSI-2 link"); > return -EINVAL; > } > @@ -438,7 +438,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->num_data_lanes) - 1; > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, > * bps = link_freq * 2 > */ > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > - do_div(mbps, priv->lanes * 1000000); > + do_div(mbps, priv->num_data_lanes * 1000000); > > return mbps; > } > > static int rcsi2_start(struct rcar_csi2 *priv) > { > - u32 phycnt, vcdt = 0, vcdt2 = 0; > + struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy; > + u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0; > struct v4l2_mbus_frame_desc fd; > unsigned int i; > int mbps, ret; > @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv) > entry->bus.csi2.channel, entry->bus.csi2.data_type); > } > > + /* Get description of the D-PHY media bus configuration. */ > + dphy = &fd.phy.csi2_dphy; > + if (dphy->clock_lane != 0) { > + dev_err(priv->dev, > + "CSI-2 configuration not supported: clock at index %u", > + dphy->clock_lane); > + return -EINVAL; > + } > + > + if (dphy->num_data_lanes > priv->available_data_lanes || > + dphy->num_data_lanes == 3) { > + dev_err(priv->dev, > + "Number of CSI-2 data lanes not supported: %u", > + dphy->num_data_lanes); > + return -EINVAL; > + } > + priv->num_data_lanes = dphy->num_data_lanes; > + > phycnt = PHYCNT_ENABLECLK; > - phycnt |= (1 << priv->lanes) - 1; > + phycnt |= (1 << priv->num_data_lanes) - 1; > > mbps = rcsi2_calc_mbps(priv, &fd); > if (mbps < 0) > @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv) > rcsi2_write(priv, VCDT_REG, vcdt); > if (vcdt2) > rcsi2_write(priv, VCDT2_REG, vcdt2); > + > /* Lanes are zero indexed. */ > - rcsi2_write(priv, LSWAP_REG, > - LSWAP_L0SEL(priv->lane_swap[0] - 1) | > - LSWAP_L1SEL(priv->lane_swap[1] - 1) | > - LSWAP_L2SEL(priv->lane_swap[2] - 1) | > - LSWAP_L3SEL(priv->lane_swap[3] - 1)); > + for (i = 0; i < priv->num_data_lanes; ++i) > + lswap |= (dphy->data_lanes[i] - 1) << (i * 2); > + rcsi2_write(priv, LSWAP_REG, lswap); > > /* Start */ > if (priv->info->init_phtw) { > @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { > static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > struct v4l2_fwnode_endpoint *vep) > { > - unsigned int i; > + unsigned int num_data_lanes; > > /* Only port 0 endpoint 0 is valid. */ > if (vep->base.port || vep->base.id) > @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > return -EINVAL; > } > > - priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > - if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > + num_data_lanes = vep->bus.mipi_csi2.num_data_lanes; > + switch (num_data_lanes) { > + case 1: > + /* fallthrough */ > + case 2: > + /* fallthrough */ > + case 4: > + priv->available_data_lanes = num_data_lanes; > + break; > + default: Nit pick, I don't like this switch statement and would prefer the original construct with an if. > dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > - priv->lanes); > + num_data_lanes); > return -EINVAL; > } > > - for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > - priv->lane_swap[i] = i < priv->lanes ? > - vep->bus.mipi_csi2.data_lanes[i] : i; > - > - /* Check for valid lane number. */ > - if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > - dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > - return -EINVAL; > - } > - } > - > return 0; > } > > @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev) > if (ret < 0) > goto error; > > - dev_info(priv->dev, "%d lanes found\n", priv->lanes); > + dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes); > > return 0; > > -- > 2.21.0 >
Hi Niklas, On Wed, Mar 20, 2019 at 08:50:15PM +0100, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your patch. > > On 2019-03-16 16:48:01 +0100, Jacopo Mondi wrote: > > Use the D-PHY configuration reported by the remote subdevice in its > > frame descriptor to configure the interface. > > > > Store the number of lanes reported through the 'data-lanes' DT property > > as the number of phyisically available lanes, which might not correspond > > to the number of lanes actually in use. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++-------- > > 1 file changed, 43 insertions(+), 28 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c > > index 6c46bcc0ee83..70b9a8165a6e 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c > > @@ -375,8 +375,8 @@ struct rcar_csi2 { > > struct mutex lock; > > int stream_count; > > > > - unsigned short lanes; > > - unsigned char lane_swap[4]; > > + unsigned short available_data_lanes; > > + unsigned short num_data_lanes; > > I don't like this, I'm sorry :-( > > I think you should keep lanes and lane_swap and most code touching them > intact. And drop num_data_lanes as it's only used when starting and only > valid for one 'session' and should not be cached in the data structure > unnecessary. > > Furthermore I think involving lane swapping in the runtime configurable > parameters is a bad idea. Or do you see a scenario where lanes could be > swapped in runtime? In my view DT should describe which physical lanes > are connected and how. And the runtime configuration part should only > deal with how many lanes are used for the particular capture session. > Yeah, it's dumb, it was noted in the review of the framework side as well.. I'll drop anything related to lane re-ordering, so I should not need to touch 'lanes' and 'lane_swap'. I need 'num_data_lanes' in 'struct rcar_csi2' though, as it is used in a few function called by rcsi2_start(). Thanks j > > }; > > > > static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) > > @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv, > > if (ret) > > return -ENODEV; > > > > - if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { > > + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) { > > dev_err(priv->dev, "Frame desc do not describe CSI-2 link"); > > return -EINVAL; > > } > > @@ -438,7 +438,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->num_data_lanes) - 1; > > > > if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && > > (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) > > @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, > > * bps = link_freq * 2 > > */ > > mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; > > - do_div(mbps, priv->lanes * 1000000); > > + do_div(mbps, priv->num_data_lanes * 1000000); > > > > return mbps; > > } > > > > static int rcsi2_start(struct rcar_csi2 *priv) > > { > > - u32 phycnt, vcdt = 0, vcdt2 = 0; > > + struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy; > > + u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0; > > struct v4l2_mbus_frame_desc fd; > > unsigned int i; > > int mbps, ret; > > @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv) > > entry->bus.csi2.channel, entry->bus.csi2.data_type); > > } > > > > + /* Get description of the D-PHY media bus configuration. */ > > + dphy = &fd.phy.csi2_dphy; > > + if (dphy->clock_lane != 0) { > > + dev_err(priv->dev, > > + "CSI-2 configuration not supported: clock at index %u", > > + dphy->clock_lane); > > + return -EINVAL; > > + } > > + > > + if (dphy->num_data_lanes > priv->available_data_lanes || > > + dphy->num_data_lanes == 3) { > > + dev_err(priv->dev, > > + "Number of CSI-2 data lanes not supported: %u", > > + dphy->num_data_lanes); > > + return -EINVAL; > > + } > > + priv->num_data_lanes = dphy->num_data_lanes; > > + > > phycnt = PHYCNT_ENABLECLK; > > - phycnt |= (1 << priv->lanes) - 1; > > + phycnt |= (1 << priv->num_data_lanes) - 1; > > > > mbps = rcsi2_calc_mbps(priv, &fd); > > if (mbps < 0) > > @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv) > > rcsi2_write(priv, VCDT_REG, vcdt); > > if (vcdt2) > > rcsi2_write(priv, VCDT2_REG, vcdt2); > > + > > /* Lanes are zero indexed. */ > > - rcsi2_write(priv, LSWAP_REG, > > - LSWAP_L0SEL(priv->lane_swap[0] - 1) | > > - LSWAP_L1SEL(priv->lane_swap[1] - 1) | > > - LSWAP_L2SEL(priv->lane_swap[2] - 1) | > > - LSWAP_L3SEL(priv->lane_swap[3] - 1)); > > + for (i = 0; i < priv->num_data_lanes; ++i) > > + lswap |= (dphy->data_lanes[i] - 1) << (i * 2); > > + rcsi2_write(priv, LSWAP_REG, lswap); > > > > /* Start */ > > if (priv->info->init_phtw) { > > @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { > > static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > > struct v4l2_fwnode_endpoint *vep) > > { > > - unsigned int i; > > + unsigned int num_data_lanes; > > > > /* Only port 0 endpoint 0 is valid. */ > > if (vep->base.port || vep->base.id) > > @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, > > return -EINVAL; > > } > > > > - priv->lanes = vep->bus.mipi_csi2.num_data_lanes; > > - if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { > > + num_data_lanes = vep->bus.mipi_csi2.num_data_lanes; > > + switch (num_data_lanes) { > > + case 1: > > + /* fallthrough */ > > + case 2: > > + /* fallthrough */ > > + case 4: > > + priv->available_data_lanes = num_data_lanes; > > + break; > > + default: > > Nit pick, I don't like this switch statement and would prefer the > original construct with an if. > > > dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", > > - priv->lanes); > > + num_data_lanes); > > return -EINVAL; > > } > > > > - for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { > > - priv->lane_swap[i] = i < priv->lanes ? > > - vep->bus.mipi_csi2.data_lanes[i] : i; > > - > > - /* Check for valid lane number. */ > > - if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { > > - dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); > > - return -EINVAL; > > - } > > - } > > - > > return 0; > > } > > > > @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev) > > if (ret < 0) > > goto error; > > > > - dev_info(priv->dev, "%d lanes found\n", priv->lanes); > > + dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes); > > > > return 0; > > > > -- > > 2.21.0 > > > > -- > Regards, > Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c index 6c46bcc0ee83..70b9a8165a6e 100644 --- a/drivers/media/platform/rcar-vin/rcar-csi2.c +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c @@ -375,8 +375,8 @@ struct rcar_csi2 { struct mutex lock; int stream_count; - unsigned short lanes; - unsigned char lane_swap[4]; + unsigned short available_data_lanes; + unsigned short num_data_lanes; }; static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd) @@ -424,7 +424,7 @@ static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv, if (ret) return -ENODEV; - if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) { + if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2_DPHY) { dev_err(priv->dev, "Frame desc do not describe CSI-2 link"); return -EINVAL; } @@ -438,7 +438,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->num_data_lanes) - 1; if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL) && (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask) @@ -511,14 +511,15 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, * bps = link_freq * 2 */ mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp; - do_div(mbps, priv->lanes * 1000000); + do_div(mbps, priv->num_data_lanes * 1000000); return mbps; } static int rcsi2_start(struct rcar_csi2 *priv) { - u32 phycnt, vcdt = 0, vcdt2 = 0; + struct v4l2_mbus_frame_desc_entry_csi2_dphy *dphy; + u32 phycnt, vcdt = 0, vcdt2 = 0, lswap = 0; struct v4l2_mbus_frame_desc fd; unsigned int i; int mbps, ret; @@ -548,8 +549,26 @@ static int rcsi2_start(struct rcar_csi2 *priv) entry->bus.csi2.channel, entry->bus.csi2.data_type); } + /* Get description of the D-PHY media bus configuration. */ + dphy = &fd.phy.csi2_dphy; + if (dphy->clock_lane != 0) { + dev_err(priv->dev, + "CSI-2 configuration not supported: clock at index %u", + dphy->clock_lane); + return -EINVAL; + } + + if (dphy->num_data_lanes > priv->available_data_lanes || + dphy->num_data_lanes == 3) { + dev_err(priv->dev, + "Number of CSI-2 data lanes not supported: %u", + dphy->num_data_lanes); + return -EINVAL; + } + priv->num_data_lanes = dphy->num_data_lanes; + phycnt = PHYCNT_ENABLECLK; - phycnt |= (1 << priv->lanes) - 1; + phycnt |= (1 << priv->num_data_lanes) - 1; mbps = rcsi2_calc_mbps(priv, &fd); if (mbps < 0) @@ -566,12 +585,11 @@ static int rcsi2_start(struct rcar_csi2 *priv) rcsi2_write(priv, VCDT_REG, vcdt); if (vcdt2) rcsi2_write(priv, VCDT2_REG, vcdt2); + /* Lanes are zero indexed. */ - rcsi2_write(priv, LSWAP_REG, - LSWAP_L0SEL(priv->lane_swap[0] - 1) | - LSWAP_L1SEL(priv->lane_swap[1] - 1) | - LSWAP_L2SEL(priv->lane_swap[2] - 1) | - LSWAP_L3SEL(priv->lane_swap[3] - 1)); + for (i = 0; i < priv->num_data_lanes; ++i) + lswap |= (dphy->data_lanes[i] - 1) << (i * 2); + rcsi2_write(priv, LSWAP_REG, lswap); /* Start */ if (priv->info->init_phtw) { @@ -822,7 +840,7 @@ static const struct v4l2_async_notifier_operations rcar_csi2_notify_ops = { static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, struct v4l2_fwnode_endpoint *vep) { - unsigned int i; + unsigned int num_data_lanes; /* Only port 0 endpoint 0 is valid. */ if (vep->base.port || vep->base.id) @@ -833,24 +851,21 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv, return -EINVAL; } - priv->lanes = vep->bus.mipi_csi2.num_data_lanes; - if (priv->lanes != 1 && priv->lanes != 2 && priv->lanes != 4) { + num_data_lanes = vep->bus.mipi_csi2.num_data_lanes; + switch (num_data_lanes) { + case 1: + /* fallthrough */ + case 2: + /* fallthrough */ + case 4: + priv->available_data_lanes = num_data_lanes; + break; + default: dev_err(priv->dev, "Unsupported number of data-lanes: %u\n", - priv->lanes); + num_data_lanes); return -EINVAL; } - for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) { - priv->lane_swap[i] = i < priv->lanes ? - vep->bus.mipi_csi2.data_lanes[i] : i; - - /* Check for valid lane number. */ - if (priv->lane_swap[i] < 1 || priv->lane_swap[i] > 4) { - dev_err(priv->dev, "data-lanes must be in 1-4 range\n"); - return -EINVAL; - } - } - return 0; } @@ -1235,7 +1250,7 @@ static int rcsi2_probe(struct platform_device *pdev) if (ret < 0) goto error; - dev_info(priv->dev, "%d lanes found\n", priv->lanes); + dev_info(priv->dev, "%d lanes found\n", priv->available_data_lanes); return 0;
Use the D-PHY configuration reported by the remote subdevice in its frame descriptor to configure the interface. Store the number of lanes reported through the 'data-lanes' DT property as the number of phyisically available lanes, which might not correspond to the number of lanes actually in use. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-csi2.c | 71 +++++++++++++-------- 1 file changed, 43 insertions(+), 28 deletions(-)