Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Sakari Ailus <sakari.ailus@iki.fi>
To: Robert Foss <robert.foss@linaro.org>
Cc: "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Dongchun Zhu <dongchun.zhu@mediatek.com>,
	Tomasz Figa <tfiga@chromium.org>,
	Maxime Ripard <maxime@cerno.tech>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Fabio Estevam <festevam@gmail.com>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE"
	<linux-arm-kernel@lists.infradead.org>,
	linux-media <linux-media@vger.kernel.org>
Subject: Re: [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings
Date: Tue, 7 Apr 2020 19:39:16 +0300
Message-ID: <20200407163916.GL6127@valkosipuli.retiisi.org.uk> (raw)
In-Reply-To: <CAG3jFysSrZJRE2TvL0bWoRFNnscgDGj8yGr-iwWBm4=1wMbJ9A@mail.gmail.com>

On Tue, Apr 07, 2020 at 05:47:41PM +0200, Robert Foss wrote:
> On Tue, 7 Apr 2020 at 14:32, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi Robert,
> >
> > On Tue, Apr 07, 2020 at 01:29:05PM +0200, Robert Foss wrote:
> > > On Tue, 7 Apr 2020 at 10:36, Maxime Ripard <maxime@cerno.tech> wrote:
> > > > On Mon, Apr 06, 2020 at 11:35:07AM +0300, Sakari Ailus wrote:
> > > > > > But that 19.2MHz is not a limitation of the device itself, it's a
> > > > > > limitation of our implementation, so we can instead implement
> > > > > > something equivalent in Linux using a clk_set_rate to 19.2MHz (to make
> > > > > > sure that our parent clock is configured at the right rate) and the
> > > > > > clk_get_rate and compare that to 19.2MHz (to make sure that it's not
> > > > > > been rounded too far apart from the frequency we expect).
> > > > > >
> > > > > > This is doing exactly the same thing, except that we don't encode our
> > > > > > implementation limitations in the DT, but in the driver instead.
> > > > >
> > > > > What I really wanted to say that a driver that doesn't get the clock
> > > > > frequency from DT but still sets that frequency is broken.
> > > > >
> > > > > This frequency is highly system specific, and in many cases only a certain
> > > > > frequency is usable, for a few reasons: On many SoCs, not all common
> > > > > frequencies can be used (e.g. 9,6 MHz, 19,2 MHz and 24 MHz; while others
> > > > > are being used as well), and then that frequency affects the usable CSI-2
> > > > > bus frequencies directly --- and of those, only safe, known-good ones
> > > > > should be used. IOW, getting the external clock frequency wrong typically
> > > > > has an effect that that none of the known-good CSI-2 bus clock frequencies
> > > > > are available.
> > > >
> > > > So clock-frequency is not about the "Frequency of the xvclk clock in
> > > > Hertz", but the frequency at which that clock must run on this
> > > > particular SoC / board to be functional?
> > > >
> > > > If so, then yeah, we should definitely keep it, but the documentation
> > > > of the binding should be made clearer as well.
> > >
> > > Alright so, let me summarise the desired approach then.
> >
> > There's a separate discussion on the same topic here:
> > https://lore.kernel.org/linux-media/20200407122106.GD4751@pendragon.ideasonboard.com/
> 
> Thanks for the link.
> 
> >
> > > ACPI:
> > >   - Fetch the "clock-frequency" property
> > >   - Verify it to be 19.2Mhz
> > >
> > > DT:
> > >   - Fetch the "clock-frequency" property
> > >   - Verify it to be 19.2Mhz
> > >   - Get xvclk clock
> > >   - Get xvclk clock rate
> > >   - Verify xvclk clock rate to be 19.2Mhz
> >
> > The current status is that you should
> > 's/clock-frequency/link-frequencies/', and in order to replace
> > assigned-clock-rates, you'll want to have a clk_set_rate to 19.2MHz
> > between steps 3 and 4
> 
> Would we want to 's/clock-frequency/link-frequencies/' for ACPI too?
> I imagine that would cause some breakage.

It would, yes, and it would be no more correct on DT either.

There are basically two possibilities here; either use the clock-frequency
property and set the frequency, or rely on assigned-clock-rates, and get
the frequency instead.

The latter, while I understand it is generally preferred, comes with having
to figure out the register list set that closest matches the frequency
obtained. The former generally gets around this silently by the clock
driver setting the closest frequency it can support.

-- 
Regards,

Sakari Ailus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-31 13:33 [PATCH v3 0/3] media: ov8856: Add devicetree support Robert Foss
2020-03-31 13:33 ` [PATCH v6 1/3] media: dt-bindings: ov8856: Document YAML bindings Robert Foss
2020-03-31 15:12   ` Marco Felsch
2020-04-02  9:57     ` Robert Foss
2020-04-03 19:21       ` Marco Felsch
2020-04-01  8:07   ` Maxime Ripard
2020-04-02 10:10     ` Robert Foss
2020-04-03 23:27       ` Sakari Ailus
2020-04-04  9:34         ` Maxime Ripard
2020-04-06  8:25           ` Robert Foss
2020-04-06  8:35           ` Sakari Ailus
2020-04-07  8:36             ` Maxime Ripard
2020-04-07 11:29               ` Robert Foss
2020-04-07 12:32                 ` Maxime Ripard
2020-04-07 15:47                   ` Robert Foss
2020-04-07 16:39                     ` Sakari Ailus [this message]
2020-04-07 16:46                       ` Tomasz Figa
2020-04-07 17:20                         ` Sakari Ailus
2020-04-08 12:21                           ` Maxime Ripard
2020-04-08 12:35                             ` Tomasz Figa
2020-04-08 13:43                               ` Maxime Ripard
2020-04-08 15:28                                 ` Sakari Ailus
2020-04-08 15:30                                   ` Sakari Ailus
2020-04-08 16:34                                     ` Andy Shevchenko
2020-04-15 10:18                                     ` Maxime Ripard
2020-04-15 11:10                                       ` Robert Foss
2020-04-15 16:16                                       ` Sakari Ailus
2020-04-20 15:02                                         ` Maxime Ripard
2020-04-09  8:32                                   ` Robert Foss
2020-04-07 16:20                   ` Sakari Ailus
2020-04-04  9:23       ` Maxime Ripard
2020-03-31 13:33 ` [PATCH v3 2/3] media: ov8856: Add devicetree support Robert Foss
2020-03-31 14:01   ` Andy Shevchenko
2020-04-06 13:37     ` Robert Foss
2020-04-06 15:06       ` Andy Shevchenko
2020-04-06 15:25         ` Robert Foss
2020-04-03 23:33   ` Sakari Ailus
2020-03-31 13:33 ` [PATCH v3 3/3] media: ov8856: Implement sensor module revision identification Robert Foss

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200407163916.GL6127@valkosipuli.retiisi.org.uk \
    --to=sakari.ailus@iki.fi \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dongchun.zhu@mediatek.com \
    --cc=festevam@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=maxime@cerno.tech \
    --cc=robert.foss@linaro.org \
    --cc=tfiga@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git