All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	robh@kernel.org
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	jbrunet@baylibre.com, mark.rutland@arm.com,
	linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org,
	mturquette@baylibre.com, carlo@caione.org, khilman@baylibre.com,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon
Date: Fri, 31 Aug 2018 10:39:00 -0700	[thread overview]
Message-ID: <153573714081.93865.1632567834341845994@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCDLWqXCcB3b3SAOxxkq+EbQazGmMVLbVLyTjWCD4wTarA@mail.gmail.com>

Quoting Martin Blumenstingl (2018-08-12 11:35:17)
> Hi Rob,
> 
> On Thu, Jul 26, 2018 at 9:32 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >
> > Hi Rob, Martin,
> >
> > On 25/07/2018 23:16, Martin Blumenstingl wrote:
> > > Hi Rob,
> > >
> > > On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
> > >>
> > >> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> > >>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> > >>> register region called "HHI". This register area contains more
> > >>> functionality than just a clock controller:
> > >>> - the clock controller
> > >>> - some reset controller bits
> > >>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> > >>> - HDMI controller
> > >>>
> > >>> The HHI register area may be accessed concurrently. Allow this by using
> > >>> a "system controller" as parent node.
> > >>
> > >> Why? A single node can be a provider of multiple things. Maybe the HDMI
> > >> should be a child since it will involve graph nodes, but the rest can be
> > >> one node. There should be numerous examples of blocks that are clock and
> > >> reset controllers.
> > > I understand that a node can provide multiple "things" - currently
> > > it's a clock controller and a reset controller
> > > the HDMI controller could also be integrated in a similar way
> > >
> > > however, I do not know how to access the temperature sensor calibration data
> > > there is an ADC - one of it's channel has access to a temperature sensor
> > > this ADC is located at CBUS offset 0x8680 and we already have a driver
> > > for it (meson-saradc)
> > > my problem is that the temperature sensor has to be calibrated - this
> > > is done by:
> > > - read data from efuse
> > > - write 4 bits of calibration data to some register in the ADC's register space
> > > - write a 5th bit of calibration data to a (seemingly random) register
> > > in the HHI register space
> > > (if one of the 5 bits is not written to it's correct location then the
> > > temperature sensor reads bogus values)
> > >
> > > I am not sure how to handle this without passing the HHI region to the
> > > meson-saradc driver and letting that initialize all the temperature
> > > calibration data bits (in it's own register space as well as the HHI
> > > register space)
> > > do you have any suggestion here?
> >
> > The clock controller node was badly represented from the beginning, the
> > HHI register zone *is* a "system control" zone with a lot of different
> > registers controller the system, including all the EE clocks, *some* HDMI
> > PHY settings, *some* Video DAC settings, *some* memory power control...
> >
> > Thus, the clock controller *is* entirely in the HHI registers space, but
> > the HHI register space should be used by other nodes for control some
> > part of the SoCs. This is why we decided to model the HHI zone as a
> > "syscon" to permit other node to have access to these registers and
> > "simple-mfd" to expose the *big* feature of the HHI : the clock controller.
> does our (Neil's and mine) explanation make sense to you?
> I understand your point that a DT node can provide multiple "things".
> this is the case on the Meson HHI region, but in addition to that the
> HHI region contains additional bits for other IP blocks. so my
> understanding is that using a syscon here is fine
> 

I'm dropping this from my review queue because nobody has replied in
many weeks. I think Rob wants people to move away from syscon so if you
can't do that then we need good reasons. I recommend putting those
reasons in the commit text and resending this series.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <sboyd@kernel.org>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	robh@kernel.org
Cc: Neil Armstrong <narmstrong@baylibre.com>,
	jbrunet@baylibre.com, mark.rutland@arm.com,
	linux-amlogic@lists.infradead.org, devicetree@vger.kernel.org,
	mturquette@baylibre.com, carlo@caione.org, khilman@baylibre.com,
	linux-clk@vger.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon
Date: Fri, 31 Aug 2018 10:39:00 -0700	[thread overview]
Message-ID: <153573714081.93865.1632567834341845994@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCDLWqXCcB3b3SAOxxkq+EbQazGmMVLbVLyTjWCD4wTarA@mail.gmail.com>

Quoting Martin Blumenstingl (2018-08-12 11:35:17)
> Hi Rob,
> =

> On Thu, Jul 26, 2018 at 9:32 AM Neil Armstrong <narmstrong@baylibre.com> =
wrote:
> >
> > Hi Rob, Martin,
> >
> > On 25/07/2018 23:16, Martin Blumenstingl wrote:
> > > Hi Rob,
> > >
> > > On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
> > >>
> > >> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> > >>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> > >>> register region called "HHI". This register area contains more
> > >>> functionality than just a clock controller:
> > >>> - the clock controller
> > >>> - some reset controller bits
> > >>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> > >>> - HDMI controller
> > >>>
> > >>> The HHI register area may be accessed concurrently. Allow this by u=
sing
> > >>> a "system controller" as parent node.
> > >>
> > >> Why? A single node can be a provider of multiple things. Maybe the H=
DMI
> > >> should be a child since it will involve graph nodes, but the rest ca=
n be
> > >> one node. There should be numerous examples of blocks that are clock=
 and
> > >> reset controllers.
> > > I understand that a node can provide multiple "things" - currently
> > > it's a clock controller and a reset controller
> > > the HDMI controller could also be integrated in a similar way
> > >
> > > however, I do not know how to access the temperature sensor calibrati=
on data
> > > there is an ADC - one of it's channel has access to a temperature sen=
sor
> > > this ADC is located at CBUS offset 0x8680 and we already have a driver
> > > for it (meson-saradc)
> > > my problem is that the temperature sensor has to be calibrated - this
> > > is done by:
> > > - read data from efuse
> > > - write 4 bits of calibration data to some register in the ADC's regi=
ster space
> > > - write a 5th bit of calibration data to a (seemingly random) register
> > > in the HHI register space
> > > (if one of the 5 bits is not written to it's correct location then the
> > > temperature sensor reads bogus values)
> > >
> > > I am not sure how to handle this without passing the HHI region to the
> > > meson-saradc driver and letting that initialize all the temperature
> > > calibration data bits (in it's own register space as well as the HHI
> > > register space)
> > > do you have any suggestion here?
> >
> > The clock controller node was badly represented from the beginning, the
> > HHI register zone *is* a "system control" zone with a lot of different
> > registers controller the system, including all the EE clocks, *some* HD=
MI
> > PHY settings, *some* Video DAC settings, *some* memory power control...
> >
> > Thus, the clock controller *is* entirely in the HHI registers space, but
> > the HHI register space should be used by other nodes for control some
> > part of the SoCs. This is why we decided to model the HHI zone as a
> > "syscon" to permit other node to have access to these registers and
> > "simple-mfd" to expose the *big* feature of the HHI : the clock control=
ler.
> does our (Neil's and mine) explanation make sense to you?
> I understand your point that a DT node can provide multiple "things".
> this is the case on the Meson HHI region, but in addition to that the
> HHI region contains additional bits for other IP blocks. so my
> understanding is that using a syscon here is fine
> =


I'm dropping this from my review queue because nobody has replied in
many weeks. I think Rob wants people to move away from syscon so if you
can't do that then we need good reasons. I recommend putting those
reasons in the commit text and resending this series.

WARNING: multiple messages have this Message-ID (diff)
From: sboyd@kernel.org (Stephen Boyd)
To: linus-amlogic@lists.infradead.org
Subject: [PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon
Date: Fri, 31 Aug 2018 10:39:00 -0700	[thread overview]
Message-ID: <153573714081.93865.1632567834341845994@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAFBinCDLWqXCcB3b3SAOxxkq+EbQazGmMVLbVLyTjWCD4wTarA@mail.gmail.com>

Quoting Martin Blumenstingl (2018-08-12 11:35:17)
> Hi Rob,
> 
> On Thu, Jul 26, 2018 at 9:32 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >
> > Hi Rob, Martin,
> >
> > On 25/07/2018 23:16, Martin Blumenstingl wrote:
> > > Hi Rob,
> > >
> > > On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
> > >>
> > >> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> > >>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> > >>> register region called "HHI". This register area contains more
> > >>> functionality than just a clock controller:
> > >>> - the clock controller
> > >>> - some reset controller bits
> > >>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> > >>> - HDMI controller
> > >>>
> > >>> The HHI register area may be accessed concurrently. Allow this by using
> > >>> a "system controller" as parent node.
> > >>
> > >> Why? A single node can be a provider of multiple things. Maybe the HDMI
> > >> should be a child since it will involve graph nodes, but the rest can be
> > >> one node. There should be numerous examples of blocks that are clock and
> > >> reset controllers.
> > > I understand that a node can provide multiple "things" - currently
> > > it's a clock controller and a reset controller
> > > the HDMI controller could also be integrated in a similar way
> > >
> > > however, I do not know how to access the temperature sensor calibration data
> > > there is an ADC - one of it's channel has access to a temperature sensor
> > > this ADC is located at CBUS offset 0x8680 and we already have a driver
> > > for it (meson-saradc)
> > > my problem is that the temperature sensor has to be calibrated - this
> > > is done by:
> > > - read data from efuse
> > > - write 4 bits of calibration data to some register in the ADC's register space
> > > - write a 5th bit of calibration data to a (seemingly random) register
> > > in the HHI register space
> > > (if one of the 5 bits is not written to it's correct location then the
> > > temperature sensor reads bogus values)
> > >
> > > I am not sure how to handle this without passing the HHI region to the
> > > meson-saradc driver and letting that initialize all the temperature
> > > calibration data bits (in it's own register space as well as the HHI
> > > register space)
> > > do you have any suggestion here?
> >
> > The clock controller node was badly represented from the beginning, the
> > HHI register zone *is* a "system control" zone with a lot of different
> > registers controller the system, including all the EE clocks, *some* HDMI
> > PHY settings, *some* Video DAC settings, *some* memory power control...
> >
> > Thus, the clock controller *is* entirely in the HHI registers space, but
> > the HHI register space should be used by other nodes for control some
> > part of the SoCs. This is why we decided to model the HHI zone as a
> > "syscon" to permit other node to have access to these registers and
> > "simple-mfd" to expose the *big* feature of the HHI : the clock controller.
> does our (Neil's and mine) explanation make sense to you?
> I understand your point that a DT node can provide multiple "things".
> this is the case on the Meson HHI region, but in addition to that the
> HHI region contains additional bits for other IP blocks. so my
> understanding is that using a syscon here is fine
> 

I'm dropping this from my review queue because nobody has replied in
many weeks. I think Rob wants people to move away from syscon so if you
can't do that then we need good reasons. I recommend putting those
reasons in the commit text and resending this series.

  reply	other threads:[~2018-08-31 17:39 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-21 19:28 [PATCH 0/3] Meson8/Meson8b: introduce a HHI syscon node Martin Blumenstingl
2018-07-21 19:28 ` Martin Blumenstingl
2018-07-21 19:28 ` [PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon Martin Blumenstingl
2018-07-21 19:28   ` Martin Blumenstingl
2018-07-25 20:07   ` Rob Herring
2018-07-25 20:07     ` Rob Herring
2018-07-25 21:16     ` Martin Blumenstingl
2018-07-25 21:16       ` Martin Blumenstingl
2018-07-26  7:32       ` Neil Armstrong
2018-07-26  7:32         ` Neil Armstrong
2018-08-12 18:35         ` Martin Blumenstingl
2018-08-12 18:35           ` Martin Blumenstingl
2018-08-31 17:39           ` Stephen Boyd [this message]
2018-08-31 17:39             ` Stephen Boyd
2018-08-31 17:39             ` Stephen Boyd
2018-09-04 12:08             ` Jerome Brunet
2018-09-04 12:08               ` Jerome Brunet
2018-07-21 19:28 ` [PATCH 2/3] clk: meson: meson8b: use the HHI syscon if available Martin Blumenstingl
2018-07-21 19:28   ` Martin Blumenstingl
2018-07-21 19:28 ` [PATCH 3/3] ARM: dts: meson: switch the clock controller to the HHI register area Martin Blumenstingl
2018-07-21 19:28   ` Martin Blumenstingl
2018-07-23  7:55 ` [PATCH 0/3] Meson8/Meson8b: introduce a HHI syscon node Neil Armstrong
2018-07-23  7:55   ` Neil Armstrong

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=153573714081.93865.1632567834341845994@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=carlo@caione.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jbrunet@baylibre.com \
    --cc=khilman@baylibre.com \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=martin.blumenstingl@googlemail.com \
    --cc=mturquette@baylibre.com \
    --cc=narmstrong@baylibre.com \
    --cc=robh@kernel.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.