Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Abel Vesa <abel.vesa@nxp.com>
To:     Stephen Boyd <sboyd@kernel.org>
CC:     Fabio Estevam <fabio.estevam@nxp.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        Mark Rutland <mark.rutland@arm.com>,
        Patrick Wildt <patrick@blueri.se>,
        Rob Herring <robh@kernel.org>,
        Sascha Hauer <kernel@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        dl-linux-imx <linux-imx@nxp.com>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and
 gaps
Thread-Topic: [PATCH] dt-bindings: clock: imx8mq: Fix numbering overlaps and
 gaps
Thread-Index: AQHU0ziyUrOqOGzNAUGITeZr+qRjmqX9Xs+AgAE2d4A=
Date:   Wed, 6 Mar 2019 13:09:42 +0000
Message-ID: <20190306130941.dqf3swx66bchm5k2@fsr-ub1664-175>
References: <1551779342-2640-1-git-send-email-abel.vesa@nxp.com>
 <155181110921.20095.1641360339603928947@swboyd.mtv.corp.google.com>
In-Reply-To: <155181110921.20095.1641360339603928947@swboyd.mtv.corp.google.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: nxp.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <ACE3C48577F71A4580F6C4435A037E26@eurprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c83256c0-5391-4657-9a8a-08d6a234ff4c
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Mar 2019 13:09:42.3602
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR04MB3956
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 19-03-05 10:38:29, Stephen Boyd wrote:
> Quoting Abel Vesa (2019-03-05 01:49:16)
> > IMX8MQ_CLK_USB_PHY_REF changes from 163 to 153, this way removing the g=
ap.
> > All the following clock ids are now decreased by 10 to keep the numberi=
ng
> > right. Doing this, the IMX8MQ_CLK_CSI2_CORE is not overlapped with
> > IMX8MQ_CLK_GPT1 anymore. IMX8MQ_CLK_GPT1_ROOT changes from 193 to 183 a=
nd
> > all the following ids are updated accordingly.
>=20
> Why do the numbers need to be consecutive? This looks difficult to merge
> given that the commit that's being "fixed" is in v5.0 and thus the
> integer value of these defines is pretty much an ABI. So if there are
> holes in the space, I suggest we leave them there unless something is
> really wrong or unworkable with that solution.
>=20

I would've ignored it but there are a few overlaps.

#define IMX8MQ_CLK_GPT1                        170
overlaps with:
#define IMX8MQ_CLK_CSI2_CORE           170

#define IMX8MQ_CLK_CSI2_PHY_REF                181
overlaps with:
#define IMX8MQ_CLK_ECSPI3_ROOT                 181

#define IMX8MQ_CLK_CSI2_ESC            182
overlaps with:
#define IMX8MQ_CLK_ENET1_ROOT                  182

By removing the gaps, some of the overlaps are also removed.

I can just get rid of the overlaps and keep the gaps if that makes it more =
ok
for the stability of the ABI.

> BTW, it would be great if the binding header was generated once and then
> never changed again so that we don't have to spend time on these sorts
> of patches in the future. Please try to fully describe each possible clk
> that might be used with a particular binding instead of adding new clk
> ids over time, especially if you have access to the silicon manufacturer
> documentation and can easily figure out all the clks that are there
> beforehand.
>=20

Here is an example of why this is not really doable: clk-sccg-pll.c.
The original design was adding the intermediary clocks like:

IMX8MQ_SYS1_PLL1_OUT       =20
IMX8MQ_SYS1_PLL1_OUT_DIV   =20
IMX8MQ_SYS1_PLL1_REF_DIV   =20
IMX8MQ_SYS1_PLL2
IMX8MQ_SYS1_PLL2_DIV =20
IMX8MQ_SYS1_PLL2_OUT =20

And these were just for SYS1_PLL. There are 2 more SYSx_PLL.
Plus the DRAM_PLL, the VIDEO2_PLL and the AUDIO_PLL.

Since the refactoring of clk-sccg-pll.c, these are not used anymore (or sho=
uld
not be used). So I would have to remove them, but as you said, it would bre=
ak
the ABI. And this example goes even further with the fact that the PHY_27M
and the mux between the PHY_27M and the OSC_27 are to be controlled by the
display controller driver itself (at this point the PHY_27M is not yet upst=
ream
and I can't say for sure it will ever be). Basically, what this means is th=
at=20
the PHY_27M will have to be exposed as a standalone clock even if it is hid=
den
behind a mux which has another clock that can provide the same rate. That
is only because there is some difference in phase (AFAIU) between the OSC_2=
7M
and the PHY_27M, at the same rate. And this is just one example.

Point being, there is no way of knowing beforehand what intermediary clocks
are needed, even with the silicon manufacturer documentation, until the
driver that makes use of a specific clock actually works.=