Hi Niklas,
On Friday, 27 July 2018 14:51:40 EEST Niklas Söderlund wrote:
> On 2018-07-27 12:25:13 +0300, Laurent Pinchart wrote:
> > On Friday, 27 July 2018 01:36:57 EEST Niklas Söderlund wrote:
> >> Latest errata document updates the start procedure for V3M. This change
> >> in addition to adhering to the datasheet update fixes capture on early
> >> revisions of V3M.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> >> ---
> >>
> >> drivers/media/platform/rcar-vin/rcar-csi2.c | 20 ++++++++++++++------
> >> 1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> ---
> >>
> >> Hi Hans, Mauro and Sakari
> >>
> >> I know this is late for v4.19 but if possible can it be considered? It
> >> fixes a real issue on R-Car V3M boards. I'm sorry for the late
> >> submission, the errata document accesses unfortunate did not align with
> >> the release schedule.
> >>
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c index
> >> daef72d410a3425d..dc5ae8025832ab6e 100644
> >> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> @@ -339,6 +339,7 @@ enum rcar_csi2_pads {
> >>
> >> struct rcar_csi2_info {
> >> int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
> >> + int (*confirm_start)(struct rcar_csi2 *priv);
> >> const struct rcsi2_mbps_reg *hsfreqrange;
> >> unsigned int csi0clkfreqrange;
> >> bool clear_ulps;
> >> @@ -545,6 +546,13 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> >> if (ret)
> >> return ret;
> >>
> >> + /* Confirm start */
> >> + if (priv->info->confirm_start) {
> >> + ret = priv->info->confirm_start(priv);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >
> > While PHTW has to be written in the "Confirmation of PHY start" sequence,
> > the operation doesn't seem to be related to confirmation of PHY start, it
> > instead looks like a shuffle of the configuration sequence. I would thus
> > not name the operation .confirm_start() as that's not what it does.
>
> I think the hook name being .confirm_start() is good as it is where in
> stream start procedure it is called. What the operation do in the V3M
> case is for me hidden as the datasheet only lists register writes and
> instructs you to check what I believe is the result of each "operation".
The PHTW register is used to communicate with the PHY through an internal bus
(could it be I2C ? That would be similar to what a well known HDMI TX IP
provider does). I believe that the read following the write is just a way to
way for the internal write to complete. The whole sequence is thus in my
opinion really a set of writes to PHY registers, and thus a configuration
sequence. I don't have more insight on this particular IP core though, all
this is based on a combination of knowledge of other multimedia IPs and some
guesswork. It would be worth asking Renesas for clarification.
> For all I know it might be a configuration sequence or a method to
> confirm that the stream is started. Do you have anymore insight to what
> it does? All I know is prior to datasheet v1.0 it was not documented for
> V3M and streaming worked fine without it, and still do.
>
> >> /* Clear Ultra Low Power interrupt. */
> >> if (priv->info->clear_ulps)
> >> rcsi2_write(priv, INTSTATE_REG,
> >> @@ -880,6 +888,11 @@ static int rcsi2_init_phtw_h3_v3h_m3n(struct
> >> rcar_csi2 *priv, unsigned int mbps)
> >> }
> >>
> >> static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2 *priv, unsigned int
> >> mbps)
> >> +{
> >> + return rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
> >> +}
> >> +
> >> +static int rcsi2_confirm_start_v3m_e3(struct rcar_csi2 *priv)
> >> {
> >> static const struct phtw_value step1[] = {
> >> { .data = 0xed, .code = 0x34 },
> >> @@ -890,12 +903,6 @@ static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2
> >> *priv, unsigned int mbps) { /* sentinel */
> >> },
> >> };
> >>
> >> - int ret;
> >> -
> >> - ret = rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
> >> - if (ret)
> >> - return ret;
> >> -
> >
> > There's something I don't get here. According to the errata, it's the
> > step1 array write sequence that need to be moved from "Start of PHY" to
> > "Confirmation of PHY start". This patch moves the PHTW frequency
> > configuration instead.
>
> Is this not what this patch do? I agree the diff is hard to read. The
> result however is more clear.
Of course, my bad. I had missed the fact that the patch also turned
rcsi2_init_phtw_v3m_e3() into rcsi2_confirm_start_v3m_e3(). Sorry for the
noise.
Given that the operation name isn't a blocker as it can be renamed later if
needed,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> static int rcsi2_init_phtw_v3m_e3(struct rcar_csi2 *priv, unsigned int
> mbps) {
> return rcsi2_phtw_write_mbps(priv, mbps, phtw_mbps_v3m_e3, 0x44);
> }
>
> static int rcsi2_confirm_start_v3m_e3(struct rcar_csi2 *priv)
> {
> static const struct phtw_value step1[] = {
> { .data = 0xed, .code = 0x34 },
> { .data = 0xed, .code = 0x44 },
> { .data = 0xed, .code = 0x54 },
> { .data = 0xed, .code = 0x84 },
> { .data = 0xed, .code = 0x94 },
> { /* sentinel */ },
> };
>
> return rcsi2_phtw_write_array(priv, step1);
> }
>
> ...
>
> static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> .init_phtw = rcsi2_init_phtw_v3m_e3,
> .confirm_start = rcsi2_confirm_start_v3m_e3,
> };
>
> >> return rcsi2_phtw_write_array(priv, step1);
> >> }
> >>
> >> @@ -949,6 +956,7 @@ static const struct rcar_csi2_info
> >> rcar_csi2_info_r8a77965 = {
> >>
> >> static const struct rcar_csi2_info rcar_csi2_info_r8a77970 = {
> >> .init_phtw = rcsi2_init_phtw_v3m_e3,
> >> + .confirm_start = rcsi2_confirm_start_v3m_e3,
> >> };
> >>
> >> static const struct of_device_id rcar_csi2_of_table[] = {
--
Regards,
Laurent Pinchart
