Message-ID: <cb54630ec2cebe6cf4707be2d41bcd93bd7a9c62.camel@nxp.com>
Subject: Re: [PATCH] driver: bus: simple-pm-bus: Add Freescale i.MX8qm/qxp
 CSR compatible strings
From:   Liu Ying <victor.liu@nxp.com>
To:     Geert Uytterhoeven <geert@linux-m68k.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        saravanak@google.com
Cc:     linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org,
        linux-imx@nxp.com, Rob Herring <robh@kernel.org>,
        Lee Jones <lee@kernel.org>, krzysztof.kozlowski+dt@linaro.org,
        robh+dt@kernel.org, Linux PM list <linux-pm@vger.kernel.org>
Date:   Tue, 31 Jan 2023 13:26:33 +0800
In-Reply-To: <CAMuHMdV0EJP5UXri3phWsCDRaoXuB=ZWK=ibwd6i-V=9f_bLkA@mail.gmail.com>
References: <20230125083248.1305270-1-victor.liu@nxp.com>
         <CAMuHMdU0KaRyP0qxJLwit1G=7zMr1k5wpCkG-wD31958mrKRWg@mail.gmail.com>
         <7c84cbd1808dc89f9ddaece13f846c4fbd12935a.camel@nxp.com>
         <eea19462-8dfa-8e10-2638-70f6f1ecc193@linaro.org>
         <2d06468ab1274af3c84e61dbca8d172b9d64cd68.camel@nxp.com>
         <f551d663-f7b6-5ae3-89a9-af1ae0d6216e@linaro.org>
         <CAMuHMdV0EJP5UXri3phWsCDRaoXuB=ZWK=ibwd6i-V=9f_bLkA@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q1ErQ2xhSitRNnFmZ3hzL1BiY3JDMTZ2aVlQL2N3M2lMOW9tYXJZbnNlYy9m?=
 =?utf-8?B?Y01NRUlrUkZYVXRBUnQ1dWJLRllCT002Mzc4dFQxY1F2QUZrait3bjcwd0tu?=
 =?utf-8?B?UE4rUkFkYU5ab2xpNXVEWE9MdDVFQ0psSWZWTFRnbTBuWGNWZlFGZ0lHblJO?=
 =?utf-8?B?T1JJcWZEbnFRZmtFYWkzd1FRZE1wYUpKRGlpYm5oRFVxWTBvaDE5Y0EvT2xJ?=
 =?utf-8?B?b3N3a0xSTFdWank0U2JJOEkrUHA4V3BCR3hwMmpRbTlxdEdzVFJOWU5kVjlO?=
 =?utf-8?B?SGhpNXlWVjlnRGpXTFZybThjUkxRcTdCNWdnUmlSV1N2eWhNYXJlUi9RMmhh?=
 =?utf-8?B?dkFtRndIOXI0WW1uUi9pRDM2TWxkS29LYSs4aTZBbUdtaXdzNTc0NkprcFlr?=
 =?utf-8?B?TXBKYkNVdFY1Mi9Uc09RNkpCL0JYcEdPRHBDZ1k4aWkvTEZwcHUxM0Nwektz?=
 =?utf-8?B?UU5yYUxsdUR6QzNNaWxDQmMrL2VlWFk0S0Q1eG5odEhBK0o1K0pVMSt3Zmtq?=
 =?utf-8?B?bExqZmF5Tm1VdGRCRmtTUFhKV1JxSVRnWkcrZGZ3UE4vUk1NOFRadDA3OHFs?=
 =?utf-8?B?a2ZaQWI2L1Ficm5FVGxSdnUvZjU0N3Bkb3RDUklKTEx1cS8yWXlkamV0K245?=
 =?utf-8?B?NEVmbS9sNmhlMi9oMHc2WTZ0clN3THRjZXlNaGZVZW5JRGJwbUtjdUZCYllK?=
 =?utf-8?B?QmNmMG1nT2lwTjNCVG5MNFYzVGQ5R01DL2RJeFZML0ROWHpXMDQwMnlHQUdU?=
 =?utf-8?B?RElpdTVjK3ZKSnNieWdJUXd1eFgzbkFPUnlpNnViMjRXMlVBbERoYjJnRlM3?=
 =?utf-8?B?aFpCOXNnTlVadDN5ZVkrR0FHYlRENnRzcmdmRkxUMXc4NDFzUmxXcmRlTllD?=
 =?utf-8?B?Y0NQMzBpaWNWeEdmZ3RkQXBxU1Y1WWU1YklEKzZXWWJwMSt1cktBcXlHUmw2?=
 =?utf-8?B?YllzWnpqOGFUbm5jZGpiQlhyZ2Y0dUpFVXhNT29GaUFQREwwNTljSUxtMjFD?=
 =?utf-8?B?cEdueklyd3IvRzZqNlBEVFJyRGdNRjQyRXdxb1VyTmRGUlUvNHVhL1pNZ21V?=
 =?utf-8?B?U3A4S3ZXd1NEaG1URjJwVTN4OEY3V1pHeGlJNjV5SkNxVm1PbjJHeThONzhv?=
 =?utf-8?B?bXBabExXMFgrZmNhQVBXVHZwUi91Znhpb2J1allqZXRkbGU1YVROcUttczZW?=
 =?utf-8?B?TGowaENna3pTNjlZQjVkUVFZV2ZtSUFsaVgrdzNjUG5NcUdEem5CdVYwRWNq?=
 =?utf-8?B?bERaTFN5NjJRMzN1cjh2MzdsMUdlR0w0d25CSWVYR0R1M3ZYc1loK0oyVzdi?=
 =?utf-8?B?eVBWSFpZWHBGYmhacktYZFdZUVhFZm81V1ZVVVRWZ0NhZzZ6Sk9VN2NmU1RN?=
 =?utf-8?B?aGs4aDc1cUlibko1KzcyNGcxTDRmMzBra0JJN0lPY1RUdDFralNTTm1yNFUx?=
 =?utf-8?B?T1hsVTR5bitQUnBnMlRSSFpFSExlSW9NQ01ucEJpaTNvZi9JTHRhd1UrYkNm?=
 =?utf-8?B?ZEN3YXM5T0pYUXhEcllRNzdwekRtZ2V2aFpERmxzS05LUGRjSjdpNzFha1ND?=
 =?utf-8?B?cE54VENpcjBjSHVUZFBtY1J3S1RUV0VlRHZ3cXB2SUU4bFZnTGg3NUhnOFNu?=
 =?utf-8?B?Vk1aenp4NHpOd2w5aFZBbW9aeU1Xcm95eDNJWW52eVB1M2lSRWoydHgxd0xG?=
 =?utf-8?B?KzExSzNDTisydktUOWV4bHJENGFMSWdWRzNuQkEvam90alMrRDhGRHlzWEdF?=
 =?utf-8?B?MzlHajZ3YW13RWZPNXNmVjVueWIxUDVxTTdhbjVPWkE2YWtyVVRvQkY5alFY?=
 =?utf-8?B?TndndjIzdGE3TU01V25GSFhQZ2ZuaEgvdGptbzVpcnNicDJNMFU1RmY3L2pT?=
 =?utf-8?B?ZHpQSkQ1dE5IaFkydkVTRDlmMllrbEhnS2JGMWJLVnlka3lVcTVVUlZlb1dK?=
 =?utf-8?B?MEFscGxyN29mbjducE9jckY5ZDhiTnUvc2gzWEtFWkxQTnhwL1RGUHVFVldt?=
 =?utf-8?B?OTl1R0ZZZm03czZFa24zUEs4bjU2a0xPUmhadjYrUUZmN3Zsd1BTNGJ3M1o3?=
 =?utf-8?B?eUhhYWkyUUg2NS9JUjM4am1Tazdub0lJemRYT2swTnkxY0JiTGZuWnF4Vlow?=
 =?utf-8?Q?/j8yS/uu1WhTjZAWDKFhLsrcT?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 691d9471-dcb7-40ff-82ec-08db034bca86
X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB7046.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2023 05:27:06.5622
 (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-CrossTenant-UserPrincipalName: 2H1hIggfle0gQtI/MFED+/rinJVLwit8SpxZ0B8J7rKju//sqn6H9M89UHgT4eUrfldBwiHRTEfbwYndXx3juA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBAPR04MB7477
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Mon, 2023-01-30 at 09:13 +0100, Geert Uytterhoeven wrote:
> Hi Krzysztof,
> 
> On Sun, Jan 29, 2023 at 11:49 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> > On 29/01/2023 09:13, Liu Ying wrote:
> > > On Thu, 2023-01-26 at 13:45 +0100, Krzysztof Kozlowski wrote:
> > > > On 26/01/2023 03:54, Liu Ying wrote:
> > > > > On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> > > > > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <
> > > > > > victor.liu@nxp.com>
> > > > > > wrote:
> > > > > > > Freescale i.MX8qm/qxp CSR module matches with what the
> > > > > > > simple
> > > > > > > power
> > > > > > > managed bus driver does, considering it needs an IPG
> > > > > > > clock to
> > > > > > > be
> > > > > > > enabled before accessing it's child devices, the child
> > > > > > > devices
> > > > > > > need
> > > > > > > to be populated by the CSR module and the child devices'
> > > > > > > power
> > > > > > > management operations need to be propagated to their
> > > > > > > parent
> > > > > > > devices.
> > > > > > > Add the CSR module's compatible strings to
> > > > > > > simple_pm_bus_of_match[]
> > > > > > > table to support the CSR module.
> > > > > > > 
> > > > > > > Suggested-by: Rob Herring <robh@kernel.org>
> > > > > > > Suggested-by: Lee Jones <lee@kernel.org>
> > > > > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > > > > The CSR module's dt-binding documentation can be found at
> > > > > > > Documentation/devicetree/bindings/mfd/fsl,imx8qxp-
> > > > > > > csr.yaml.
> > > > > > > 
> > > > > > > Suggested by Rob and Lee in this thread:
> > > > > 
> > > 
> > > 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fproject%2Flinux-arm-kernel%2Fpatch%2F20221017075702.4182846-1-victor.liu%40nxp.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7C721a36c64fab482d5d3908db0299e1d8%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638106632175447990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=jnxUn2N33xjrmNfGXw02SeG5EN%2Fqluz%2BjZYRk0%2BHlrU%3D&reserved=0
> > > > > > > 
> > > > > > >  drivers/bus/simple-pm-bus.c | 2 ++
> > > > > > >  1 file changed, 2 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/bus/simple-pm-bus.c
> > > > > > > b/drivers/bus/simple-
> > > > > > > pm-
> > > > > > > bus.c
> > > > > > > index 7afe1947e1c0..4a7575afe6c6 100644
> > > > > > > --- a/drivers/bus/simple-pm-bus.c
> > > > > > > +++ b/drivers/bus/simple-pm-bus.c
> > > > > > > @@ -120,6 +120,8 @@ static const struct of_device_id
> > > > > > > simple_pm_bus_of_match[] = {
> > > > > > >         { .compatible = "simple-mfd",   .data = ONLY_BUS
> > > > > > > },
> > > > > > >         { .compatible = "isa",          .data = ONLY_BUS
> > > > > > > },
> > > > > > >         { .compatible = "arm,amba-bus", .data = ONLY_BUS
> > > > > > > },
> > > > > > > +       { .compatible = "fsl,imx8qm-lvds-csr", },
> > > > > > > +       { .compatible = "fsl,imx8qxp-mipi-lvds-csr", },
> > > > > > 
> > > > > > I did read the thread linked above, and I still think you
> > > > > > should
> > > > > > just
> > > > > > add "simple-pm-bus" to the compatible value in DTS, so no
> > > > > > driver
> > > > > > change
> > > > > > is needed, cfr.
> > > > > > Documentation/devicetree/bindings/bus/renesas,bsc.yaml.
> > > > 
> > > > I don't think we want to start putting specific compatibles
> > > > here. We
> > > > don't do it for simple-mfd, syscon and simple-bus, so neither
> > > > should
> > > > we
> > > > do it here.
> > > > 
> > > > > This means that i.MX8qm/qxp CSR module dt-binding
> > > > > documentation
> > > > > needs
> > > > > to be changed.  I'd like to know how Rob and Krzysztof think
> > > > > about
> > > > > that.
> > > > 
> > > > The fsl,imx8qxp-csr.yaml bindings are broken anyway... You have
> > > > device
> > > > specific bindings for non-simple device but use simple-mfd. You
> > > > cannot.
> > > > simple-mfd means it is simple and none of the resources are
> > > > needed
> > > > for
> > > > children, but that binding contradicts it.
> > > > 
> > > > Now you kind of try to extend it even more make it more and
> > > > more
> > > > broken.
> > > > 
> > > > Rework the bindings keeping them backwards compatible. The
> > > > combination
> > > > with simple-mfd should be deprecated and you can add whatever
> > > > is
> > > > needed
> > > > for a proper setup.
> > > 
> > > I did try to rework the bindings and make the combination with
> > > simple-
> > > mfd deprecated. However, it reminds me the problem that "simple-
> > > pm-bus"
> > > and "syscon" can not be in compatible string at the same time,
> > > otherwise, nodename should match '^syscon@[0-9a-f]+$' and
> > > '^bus@[0-9a-
> > > f]+$' at the same time. I mentioned the problem in the same
> > > thread[1]
> > > where Rob and Lee suggest to go with this patch. "syscon" is
> > > needed
> > > since i.MX8qxp MIPI DSI/LVDS combo PHY node references the CSR
> > > module
> > > through a phandle, so dropping/deprecating "syscon" is a no-go.
> > > 
> > > Also, as Rob mentioned in [1] "if register space is all mixed
> > > together,
> > > then it is the former and an MFD", I think the CSR module should
> > > fall
> > > into the simple-mfd category.
> > 
> > You are now mixing MFD with simple-mfd. If you have clocks there or
> > any
> > other resources, it's not simple-mfd anymore.
> 
> I beg to differ (but not w.r.t. the other resources): if any "glue"
> device
> between parent and child hierarchies does not call
> pm_runtime_enable(),
> Runtime PM (which is a Linux thing, not a DT thing) for the children
> may not work correctly, as it won't propagate correctly to the
> parent.
> So IMHO this is something to fix in Linux, not in DT.
> 
> > > Take i.MX8qxp MIPI DSI/LVDS CSR module as
> > > an example, child device pxl2dpi register offset is 0x40, while
> > > child
> > > device ldb register offsets are 0x20 and 0xe0.
> > > 
> > > Geert, Krzysztof, can you please consider to keep this patch as-
> > > is,
> > > since it seems that there is no other option?
> > 
> > There are other options, why do you say there is no? Making it
> > proper
> > binding/driver for its children without abusing simple bindings.
> > Simple
> > bindings are for simple cases and this turns out not the simple
> > case.
> 
> Or drop the ".data = ONLY_BUS" for "simple-mfd"?

Saravana said it's wrong to deleting ONLY_BUS for "simple-mfd". See
Saravana's comments in [1].

[1] 
https://lore.kernel.org/linux-arm-kernel/CAGETcx_QVaYYHsD9HZmBu404K-oXRCPm4N4GRrYu4pGyw2DHbg@mail.gmail.com/

Regards,
Liu Ying

> (and treat "simple-bus" the same as "simple-pm-bus", too ;-)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- 
> geert@linux-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