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 <[email protected]>
Suggested-by: Lee Jones <[email protected]>
Signed-off-by: Liu Ying <[email protected]>
---
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://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
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", },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match);
--
2.37.1
Hi Liu,
On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]> 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 <[email protected]>
> Suggested-by: Lee Jones <[email protected]>
> Signed-off-by: Liu Ying <[email protected]>
Thanks for your patch!
> ---
> 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://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
>
> 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.
If that doesn't work due to DT binding constraints, the latter should
be fixed.
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match);
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> Hi Liu,
Hi Geert,
>
> On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]> 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 <[email protected]>
> > Suggested-by: Lee Jones <[email protected]>
> > Signed-off-by: Liu Ying <[email protected]>
>
> Thanks for your patch!
Thanks for your review!
>
> > ---
> > 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%7C3fa98ff270534078019508dafeb34b10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638102343312884116%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zm8Z5gWt9yGXakYlxopUfZKLMUJRWTxq1kWHLyqhyww%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.
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.
>
> If that doesn't work due to DT binding constraints, the latter should
> be fixed.
Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
because "simple-mfd" is matched first and "ONLY_BUS" is set for
"simple-mfd". s/simple-mfd/simple-pm-bus/ for the compatbile value in
DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
changed and moved from mfd directory to bus directory...
Hmm, fsl,imx8qxp-csr.yaml was first written earlier than the below
patch. Without that patch, child devices of the CSR module can be
populated.
98e96cf80045 ("drivers: bus: simple-pm-bus: Add support for probing
simple bus only devices")
Should I go ahead to fix fsl,imx8qxp-csr.yaml and move it to bus
directory?
Regards,
Liu Ying
>
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, simple_pm_bus_of_match);
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> 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
Hi Liu,
On Thu, Jan 26, 2023 at 3:55 AM Liu Ying <[email protected]> wrote:
> On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]> 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 <[email protected]>
> > > Suggested-by: Lee Jones <[email protected]>
> > > Signed-off-by: Liu Ying <[email protected]>
> >
> > Thanks for your patch!
>
> Thanks for your review!
>
> >
> > > ---
> > > 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%7C3fa98ff270534078019508dafeb34b10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638102343312884116%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zm8Z5gWt9yGXakYlxopUfZKLMUJRWTxq1kWHLyqhyww%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.
>
> 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.
>
> >
> > If that doesn't work due to DT binding constraints, the latter should
> > be fixed.
>
> Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
> because "simple-mfd" is matched first and "ONLY_BUS" is set for
Is that because you have both "simple-mfd" and "simple-pm-bus",
and the former is listed first in DTS?
Does it work if you change the order?
> "simple-mfd". s/simple-mfd/simple-pm-bus/ for the compatbile value in
> DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
> changed and moved from mfd directory to bus directory...
BTW, originally I didn't want to introduce "simple-pm-bus" at all,
and make it just call pm_runtime_enable() for "simple-bus" (PM is
everywhere anyway), but that was rejected...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
On Thu, 2023-01-26 at 09:30 +0100, Geert Uytterhoeven wrote:
> Hi Liu,
Hi Geert,
>
> On Thu, Jan 26, 2023 at 3:55 AM Liu Ying <[email protected]> wrote:
> > On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
> > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
> > > 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 <[email protected]>
> > > > Suggested-by: Lee Jones <[email protected]>
> > > > Signed-off-by: Liu Ying <[email protected]>
> > >
> > > Thanks for your patch!
> >
> > Thanks for your review!
> >
> > >
> > > > ---
> > > > 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%7C5121c05b779f4003da7b08daff77a9ac%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103186659610847%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=yyYJNJcWwDjZESJ%2FiAqkEhKbz2jRdDbgmQ%2Bm%2FWwdWZs%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.
> >
> > 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.
> >
> > >
> > > If that doesn't work due to DT binding constraints, the latter
> > > should
> > > be fixed.
> >
> > Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
> > because "simple-mfd" is matched first and "ONLY_BUS" is set for
>
> Is that because you have both "simple-mfd" and "simple-pm-bus",
> and the former is listed first in DTS?
Yes. For example, compatible is set as below for CSR module in i.MX8qm
LVDS subsystem.
compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-mfd", "simple-pm-
bus";
> Does it work if you change the order?
Yes, it works if I swap the positions of "simple-mfd" and "simple-pm-
bus".
Device tree specification says the compatible strings should be listed
from most specific to most general. "simple-mfd" and "simple-pm-bus"
sound like two different things, so I don't have good idea about the
order. Replacing "simple-mfd" with "simple-pm-bus" and moving
fsl,imx8qxp-csr.yaml to bus directory look more reasonable to me. But,
I'd like to know how do device tree folks think.
Regards,
Liu Ying
>
> > "simple-mfd". s/simple-mfd/simple-pm-bus/ for the compatbile value
> > in
> > DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
> > changed and moved from mfd directory to bus directory...
>
> BTW, originally I didn't want to introduce "simple-pm-bus" at all,
> and make it just call pm_runtime_enable() for "simple-bus" (PM is
> everywhere anyway), but that was rejected...
>
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> [email protected]
>
> 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
On 26/01/2023 03:54, Liu Ying wrote:
> On Wed, 2023-01-25 at 10:05 +0100, Geert Uytterhoeven wrote:
>> Hi Liu,
>
> Hi Geert,
>
>>
>> On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]> 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 <[email protected]>
>>> Suggested-by: Lee Jones <[email protected]>
>>> Signed-off-by: Liu Ying <[email protected]>
>>
>> Thanks for your patch!
>
> Thanks for your review!
>
>>
>>> ---
>>> 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%7C3fa98ff270534078019508dafeb34b10%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638102343312884116%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=zm8Z5gWt9yGXakYlxopUfZKLMUJRWTxq1kWHLyqhyww%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.
>
>>
>> If that doesn't work due to DT binding constraints, the latter should
>> be fixed.
>
> Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
> because "simple-mfd" is matched first and "ONLY_BUS" is set for
> "simple-mfd". s/simple-mfd/simple-pm-bus/ for the compatbile value in
> DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
> changed and moved from mfd directory to bus directory...
Because the device is not simple-mfd and should have never been made
that. I am surprised it passed Rob's review, I guess it slipped through
the cracks.
Now you have to live with borken bindings. You have a lesson for future
- put some effort to design them correctly from the beginning, so you
won't have problems. Bindings should be complete from the beginning, not
"I'll develop whatever is needed to match my driver and I will not care
about future".
Best regards,
Krzysztof
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:
> > > Hi Liu,
> >
> > Hi Geert,
> >
> > >
> > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
> > > 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 <[email protected]>
> > > > Suggested-by: Lee Jones <[email protected]>
> > > > Signed-off-by: Liu Ying <[email protected]>
> > >
> > > Thanks for your patch!
> >
> > Thanks for your review!
> >
> > >
> > > > ---
> > > > 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%7C87515adc8fc3401f410808daff9b3279%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103339276325657%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FFz5gSIPc6vyvb1IJ1Umu62WpzjNLIiQIi2sOA3RQGc%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. 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?
[1]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
Regards,
Liu Ying
>
> >
> > >
> > > If that doesn't work due to DT binding constraints, the latter
> > > should
> > > be fixed.
> >
> > Adding "simple-pm-bus" to the compatible value in DTS doesn't work,
> > because "simple-mfd" is matched first and "ONLY_BUS" is set for
> > "simple-mfd". s/simple-mfd/simple-pm-bus/ for the compatbile value
> > in
> > DTS does work, but it means that fsl,imx8qxp-csr.yaml needs to be
> > changed and moved from mfd directory to bus directory...
>
> Because the device is not simple-mfd and should have never been made
> that. I am surprised it passed Rob's review, I guess it slipped
> through
> the cracks.
>
> Now you have to live with borken bindings. You have a lesson for
> future
> - put some effort to design them correctly from the beginning, so you
> won't have problems. Bindings should be complete from the beginning,
> not
> "I'll develop whatever is needed to match my driver and I will not
> care
> about future".
>
> Best regards,
> Krzysztof
>
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:
>>>> Hi Liu,
>>>
>>> Hi Geert,
>>>
>>>>
>>>> On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
>>>> 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 <[email protected]>
>>>>> Suggested-by: Lee Jones <[email protected]>
>>>>> Signed-off-by: Liu Ying <[email protected]>
>>>>
>>>> Thanks for your patch!
>>>
>>> Thanks for your review!
>>>
>>>>
>>>>> ---
>>>>> 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%7C87515adc8fc3401f410808daff9b3279%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103339276325657%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FFz5gSIPc6vyvb1IJ1Umu62WpzjNLIiQIi2sOA3RQGc%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.
> 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.
Best regards,
Krzysztof
On Sun, 2023-01-29 at 11:49 +0100, Krzysztof Kozlowski 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:
> > > > > Hi Liu,
> > > >
> > > > Hi Geert,
> > > >
> > > > >
> > > > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
> > > > > 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 <[email protected]>
> > > > > > Suggested-by: Lee Jones <[email protected]>
> > > > > > Signed-off-by: Liu Ying <[email protected]>
> > > > >
> > > > > Thanks for your patch!
> > > >
> > > > Thanks for your review!
> > > >
> > > > >
> > > > > > ---
> > > > > > 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%7C58af8a86f0134b6bde3408db01e68522%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638105861813147063%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mHv%2BTAHMAR8coxDmXucoMbxv%2BuMEdHWHTyLz16OUY50%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 may try to make the combination with simple-mfd deprecated and add
another combination with i.MX8qm/qxp CSR compatible strings and syscon
only. Then, it will be a MFD, not simple-mfd.
>
> > 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.
I don't quite understand your comment here, sorry. Here are the 3
options I know:
1) Add a new MFD driver for the CSR module
I sent out a MFD driver[1] for the CSR module for review, but Rob and
Lee provided comments there and suggested to use this patch.
2) Use "simple-pm-bus" compatible string in the CSR module's compatbile
property
As mentioned before, "simple-pm-bus" contradicts with "syscon".
3) Add the CSR module's specific compatible strings in
simple_pm_bus_of_match[]
This is what this patch does and suggested by Rob and Lee.
Looks like 3) is the only feasible option.
Geert, Krzysztof, any objections to keep this patch as-is?
[1]
https://patchwork.kernel.org/project/linux-arm-kernel/patch/[email protected]/
Regards,
Liu Ying
> Simple
> bindings are for simple cases and this turns out not the simple case.
>
> Best regards,
> Krzysztof
>
Hi Krzysztof,
On Sun, Jan 29, 2023 at 11:49 AM Krzysztof Kozlowski
<[email protected]> 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 <[email protected]>
> >>>> 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 <[email protected]>
> >>>>> Suggested-by: Lee Jones <[email protected]>
> >>>>> Signed-off-by: Liu Ying <[email protected]>
> >>>>> 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%7C87515adc8fc3401f410808daff9b3279%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103339276325657%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FFz5gSIPc6vyvb1IJ1Umu62WpzjNLIiQIi2sOA3RQGc%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"?
(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 -- [email protected]
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
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
> <[email protected]> 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 <
> > > > > > [email protected]>
> > > > > > 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 <[email protected]>
> > > > > > > Suggested-by: Lee Jones <[email protected]>
> > > > > > > Signed-off-by: Liu Ying <[email protected]>
> > > > > > > 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 --
> [email protected]
>
> 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
On 30/01/2023 02:45, Liu Ying wrote:
> On Sun, 2023-01-29 at 11:49 +0100, Krzysztof Kozlowski 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:
>>>>>> Hi Liu,
>>>>>
>>>>> Hi Geert,
>>>>>
>>>>>>
>>>>>> On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
>>>>>> 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 <[email protected]>
>>>>>>> Suggested-by: Lee Jones <[email protected]>
>>>>>>> Signed-off-by: Liu Ying <[email protected]>
>>>>>>
>>>>>> Thanks for your patch!
>>>>>
>>>>> Thanks for your review!
>>>>>
>>>>>>
>>>>>>> ---
>>>>>>> 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%7C58af8a86f0134b6bde3408db01e68522%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638105861813147063%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mHv%2BTAHMAR8coxDmXucoMbxv%2BuMEdHWHTyLz16OUY50%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 may try to make the combination with simple-mfd deprecated and add
> another combination with i.MX8qm/qxp CSR compatible strings and syscon
> only. Then, it will be a MFD, not simple-mfd.
>
>>
>>> 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.
>
> I don't quite understand your comment here, sorry. Here are the 3
> options I know:
>
> 1) Add a new MFD driver for the CSR module
> I sent out a MFD driver[1] for the CSR module for review, but Rob and
> Lee provided comments there and suggested to use this patch.
Where are the clocks in that driver? Having MFD for something without
resources does not make any sense - this is why we have simple-mfd.
But I see in your [1] Rob's suggestion about adding the compatible to
simple-pm-bus.c, therefore it looks correct approach.
Best regards,
Krzysztof
On Wed, 2023-02-01 at 08:31 +0100, Krzysztof Kozlowski wrote:
> On 30/01/2023 02:45, Liu Ying wrote:
> > On Sun, 2023-01-29 at 11:49 +0100, Krzysztof Kozlowski 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:
> > > > > > > Hi Liu,
> > > > > >
> > > > > > Hi Geert,
> > > > > >
> > > > > > > On Wed, Jan 25, 2023 at 9:31 AM Liu Ying <[email protected]>
> > > > > > > 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 <[email protected]>
> > > > > > > > Suggested-by: Lee Jones <[email protected]>
> > > > > > > > Signed-off-by: Liu Ying <[email protected]>
> > > > > > >
> > > > > > > Thanks for your patch!
> > > > > >
> > > > > > Thanks for your review!
> > > > > >
> > > > > > > > ---
> > > > > > > > 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%7C32a4c39e47ec4fc834ae08db04264e35%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638108334805268216%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=M2SddU7wPx465kB%2F5p2r6j3%2FAotcVvMiPORPzh%2BC%2FxY%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 may try to make the combination with simple-mfd deprecated and add
> > another combination with i.MX8qm/qxp CSR compatible strings and syscon
> > only. Then, it will be a MFD, not simple-mfd.
> >
> > > > 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.
> >
> > I don't quite understand your comment here, sorry. Here are the 3
> > options I know:
> >
> > 1) Add a new MFD driver for the CSR module
> > I sent out a MFD driver[1] for the CSR module for review, but Rob and
> > Lee provided comments there and suggested to use this patch.
>
> Where are the clocks in that driver? Having MFD for something without
The clocks are missed in that driver. But simple-pm-bus.c in linux-next
uses the clocks.
Regards,
Liu Ying
> resources does not make any sense - this is why we have simple-mfd.
>
> But I see in your [1] Rob's suggestion about adding the compatible to
> simple-pm-bus.c, therefore it looks correct approach.
>
> Best regards,
> Krzysztof
>