Date:   Tue, 12 Jan 2021 10:10:39 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Jacopo Mondi <jacopo@...ndi.org>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Jacopo Mondi <jacopo+renesas@...ndi.org>,
        Kieran Bingham <kieran.bingham+renesas@...asonboard.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Niklas Söderlund 
        <niklas.soderlund+renesas@...natech.se>,
        Linux Media Mailing List <linux-media@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Hyun Kwon <hyunk@...inx.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
        Sergei Shtylyov <sergei.shtylyov@...il.com>
Subject: Re: [PATCH v6 5/5] media: i2c: max9286: Configure reverse channel amplitude

Hi Jacopo,

On Tue, Jan 12, 2021 at 10:07 AM Jacopo Mondi <jacopo@...ndi.org> wrote:
> On Tue, Jan 12, 2021 at 07:03:42AM +0200, Laurent Pinchart wrote:
> > On Mon, Jan 11, 2021 at 12:20:23PM +0100, Jacopo Mondi wrote:
> > > On Mon, Jan 11, 2021 at 12:58:59PM +0200, Laurent Pinchart wrote:
> > > > On Mon, Jan 11, 2021 at 11:43:11AM +0100, Jacopo Mondi wrote:
> > > >> On Wed, Dec 16, 2020 at 07:22:17PM +0200, Laurent Pinchart wrote:
> > > >>> On Tue, Dec 15, 2020 at 06:09:57PM +0100, Jacopo Mondi wrote:
> > > >>>> Adjust the initial reverse channel amplitude parsing from
> > > >>>> firmware interface the 'maxim,reverse-channel-microvolt'
> > > >>>> property.
> > > >>>>
> > > >>>> This change is required for both rdacm20 and rdacm21 camera
> > > >>>> modules to be correctly probed when used in combination with
> > > >>>> the max9286 deserializer.
> > > >>>>
> > > >>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
> > > >>>> Signed-off-by: Jacopo Mondi <jacopo+renesas@...ndi.org>
> > > >>>> ---
> > > >>>>  drivers/media/i2c/max9286.c | 23 ++++++++++++++++++++++-
> > > >>>>  1 file changed, 22 insertions(+), 1 deletion(-)
> > > >>>>
> > > >>>> diff --git a/drivers/media/i2c/max9286.c b/drivers/media/i2c/max9286.c
> > > >>>> index 021309c6dd6f..9b40a4890c4d 100644
> > > >>>> --- a/drivers/media/i2c/max9286.c
> > > >>>> +++ b/drivers/media/i2c/max9286.c
> > > >>>> @@ -163,6 +163,8 @@ struct max9286_priv {
> > > >>>>        unsigned int mux_channel;
> > > >>>>        bool mux_open;
> > > >>>>
> > > >>>> +      u32 reverse_channel_mv;
> > > >>>> +
> > > >>>>        struct v4l2_ctrl_handler ctrls;
> > > >>>>        struct v4l2_ctrl *pixelrate;
> > > >>>>
> > > >>>> @@ -557,10 +559,14 @@ static int max9286_notify_bound(struct v4l2_async_notifier *notifier,
> > > >>>>         * All enabled sources have probed and enabled their reverse control
> > > >>>>         * channels:
> > > >>>>         *
> > > >>>> +       * - Increase the reverse channel amplitude to compensate for the
> > > >>>> +       *   remote ends high threshold, if not done already
> > > >>>>         * - Verify all configuration links are properly detected
> > > >>>>         * - Disable auto-ack as communication on the control channel are now
> > > >>>>         *   stable.
> > > >>>>         */
> > > >>>> +      if (priv->reverse_channel_mv < 170)
> > > >>>> +              max9286_reverse_channel_setup(priv, 170);
> > > >>>
> > > >>> I'm beginning to wonder if there will be a need in the future to not
> > > >>> increase the reverse channel amplitude (keeping the threshold low on the
> > > >>> remote side). An increased amplitude increases power consumption, and if
> > > >>> the environment isn't noisy, a low amplitude would work. The device tree
> > > >>> would then need to specify both the initial amplitude required by the
> > > >>> remote side, and the desired amplitude after initialization. What do you
> > > >>> think ? Is it overkill ? We don't have to implement this now, so
> > > >>>
> > > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@...asonboard.com>
> > > >>>
> > > >>> but if this feature could be required later, we may want to take into
> > > >>> account in the naming of the new DT property to reflect the fact that it
> > > >>> is the initial value.
> > > >>
> > > >> I had the same thought when I initially proposed
> > > >> "maxim,initial-reverse-channel-mV"
> > > >>
> > > >> Having to use the standard unit suffix that would have become
> > > >> "maxim,initial-reverse-channel-microvolt"
> > > >> which is extremely long.
> > > >>
> > > >> I can't tell if there will be any need to adjust the amplitude later.
> > > >> In any case, I would not rely on a DTS property to do so, as once we
> > > >> have probed the remote we have a subdev where to call
> > > >> 'get_mbus_config()' on, and from there we can report the high threshold
> > > >> status of the serializer and adjust the deser amplitude accordingly.
> > > >
> > > > I don't think that's the point. The threshold of the serializer is
> > > > something we can configure at runtime. What voltage level to use after
> > >
> > > How so ? I mean, we can add an API for this, but currently it's
> > > configured at probe time and that's it. Its configuration might as
> > > well come from a DT property like we do on the deserializer here but I
> > > fail to see why it's different. Both settings depends on the required
> > > noise immunity of th system.
> >
> > The voltage level configuration need to match between the tserializer
> > (transmitter) and the deserializer (receiver). The serializer is
> > configured with a voltage level, and the deserializer needs to be
> > configured with a corresponding threshold.
> >
>
> If I'm not mistaken it's actually the other way around, at least for
> the chips we're dealing with.
>
> The serializer (MAX9271) has an "Reverse Channel Receiver High
> Threshold Enable" bit (register 0x08[0]) undocumented in the chip
> manual but described in the "MAX9286 Programming Guide 2 10.pdf"
> document in the "Important Registers" section.
>
> The deserializer (MAX9286) has instead a configurable setting for the reverse
> channel signal amplitude, which is what we are controlling in this
> series.
>
> The deserializer reverse channel amplitude has to match the remote
> side 'high threshold enable' setting. If it is enabled the amplitude
> has to be increased to be able to probe the remote side. If it's not
> a lower amplitude has to be used to make comunication reliable.
>
> As you said, some models (RDACM20) might be pre-programmed with the
> 'high threshold enable' bit set, and so the deserializer reverse
> channel amplitude has to be adjusted accordingly to be able to
> comunicate on the reverse channel.
>
> > The voltage level of the serializer is configurable on the camera side
> > when the system is powered up. The RDACM20 has a microcontroller which
> > can configure the serializer, and other cameras may have similar
> > mechanisms. As the deserializer can't query the information from the
> > serializer (communication is unreliable if the threshold has an
> > incorrect value), we need a DT property to tell the deserializer what
> > threshold is initially used by the camera when it gets powered up.
> >
>
> That's what this series does, yes.
>
> > This only covers initialization. A camera could boot up with a low
> > voltage level, but we may want to increase the voltage level (and thus
> > the threshold on the deserializer side) to increase noise immunity. Or,
> > if the system environment isn't noisy, we may want to keep a low voltage
> > level, or even decrease it if the camera boots up with a high voltage
> > level. This runtime voltage level depends on the system design and its
> > susceptibility to noise, and is thus a system property. Should we want
> > to make it configurable, it should be specified in DT, and it's separate
> > from the initial voltage level that is used to establish communication.
> >
>
> And that's what I meant. Assuming we handle initialization correctly
> with this series, the serializers 'high threshold' configuration
> -after- initialization can be specified with a DT property on the
> -serializer- side. Then, to adjust the deserializer reverse channel
> amplitude, once we the remote has probed and we have a subdevice
> registered for it, we can query the 'high threshold' configuration
> using get_mbus_config() (or another API if we think it's better) and
> adjust the deserializer accordingly.
>
> All in all:
> - yes, I think there might be a need to control the noise immunity
>   settings after initialization
> - I think it should be done on the serializer side, possibly with a DT
>   property, possibly something like a boolean 'maxim,high-threshold-enable'

Can the needed voltage level be calibrated at runtime, cfr. DDR
link training?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists