diff mbox series

[v2,1/3] arm64: imx8mq: add imx8mq iomux-gpr field defines

Message ID e0562d8bb4098dc4cdb4023b41fb75b312be22a5.1565367567.git.agx@sigxcpu.org (mailing list archive)
State New, archived
Headers show
Series drm: bridge: Add NWL MIPI DSI host controller support | expand

Commit Message

Guido Günther Aug. 9, 2019, 4:24 p.m. UTC 
This adds all the gpr registers and the define needed for selecting
the input source in the imx-nwl drm bridge.

Signed-off-by: Guido Günther <agx@sigxcpu.org>
---
 include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h | 62 ++++++++++++++++++++
 1 file changed, 62 insertions(+)
 create mode 100644 include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h

Comments

Lee Jones Aug. 12, 2019, 10:24 a.m. UTC | #1 
On Fri, 09 Aug 2019, Guido Günther wrote:

> This adds all the gpr registers and the define needed for selecting
> the input source in the imx-nwl drm bridge.
> 
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> ---
>  include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h | 62 ++++++++++++++++++++
>  1 file changed, 62 insertions(+)
>  create mode 100644 include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h

I would like Arnd to look at this please.

> diff --git a/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h b/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h
> new file mode 100644
> index 000000000000..62e85ffacfad
> --- /dev/null
> +++ b/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2017 NXP
> + *               2019 Purism SPC
> + */
> +
> +#ifndef __LINUX_IMX8MQ_IOMUXC_GPR_H
> +#define __LINUX_IMX8MQ_IOMUXC_GPR_H
> +
> +#define IOMUXC_GPR0	0x00
> +#define IOMUXC_GPR1	0x04
> +#define IOMUXC_GPR2	0x08
> +#define IOMUXC_GPR3	0x0c
> +#define IOMUXC_GPR4	0x10
> +#define IOMUXC_GPR5	0x14
> +#define IOMUXC_GPR6	0x18
> +#define IOMUXC_GPR7	0x1c
> +#define IOMUXC_GPR8	0x20
> +#define IOMUXC_GPR9	0x24
> +#define IOMUXC_GPR10	0x28
> +#define IOMUXC_GPR11	0x2c
> +#define IOMUXC_GPR12	0x30
> +#define IOMUXC_GPR13	0x34
> +#define IOMUXC_GPR14	0x38
> +#define IOMUXC_GPR15	0x3c
> +#define IOMUXC_GPR16	0x40
> +#define IOMUXC_GPR17	0x44
> +#define IOMUXC_GPR18	0x48
> +#define IOMUXC_GPR19	0x4c
> +#define IOMUXC_GPR20	0x50
> +#define IOMUXC_GPR21	0x54
> +#define IOMUXC_GPR22	0x58
> +#define IOMUXC_GPR23	0x5c
> +#define IOMUXC_GPR24	0x60
> +#define IOMUXC_GPR25	0x64
> +#define IOMUXC_GPR26	0x68
> +#define IOMUXC_GPR27	0x6c
> +#define IOMUXC_GPR28	0x70
> +#define IOMUXC_GPR29	0x74
> +#define IOMUXC_GPR30	0x78
> +#define IOMUXC_GPR31	0x7c
> +#define IOMUXC_GPR32	0x80
> +#define IOMUXC_GPR33	0x84
> +#define IOMUXC_GPR34	0x88
> +#define IOMUXC_GPR35	0x8c
> +#define IOMUXC_GPR36	0x90
> +#define IOMUXC_GPR37	0x94
> +#define IOMUXC_GPR38	0x98
> +#define IOMUXC_GPR39	0x9c
> +#define IOMUXC_GPR40	0xa0
> +#define IOMUXC_GPR41	0xa4
> +#define IOMUXC_GPR42	0xa8
> +#define IOMUXC_GPR43	0xac
> +#define IOMUXC_GPR44	0xb0
> +#define IOMUXC_GPR45	0xb4
> +#define IOMUXC_GPR46	0xb8
> +#define IOMUXC_GPR47	0xbc
> +
> +/* i.MX8Mq iomux gpr register field defines */
> +#define IMX8MQ_GPR13_MIPI_MUX_SEL		BIT(2)
> +
> +#endif /* __LINUX_IMX8MQ_IOMUXC_GPR_H */
Arnd Bergmann Aug. 13, 2019, 8:08 a.m. UTC | #2 
On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
>
> This adds all the gpr registers and the define needed for selecting
> the input source in the imx-nwl drm bridge.
>
> Signed-off-by: Guido Günther <agx@sigxcpu.org>
> +
> +#define IOMUXC_GPR0    0x00
> +#define IOMUXC_GPR1    0x04
> +#define IOMUXC_GPR2    0x08
> +#define IOMUXC_GPR3    0x0c
> +#define IOMUXC_GPR4    0x10
> +#define IOMUXC_GPR5    0x14
> +#define IOMUXC_GPR6    0x18
> +#define IOMUXC_GPR7    0x1c
(more of the same)

huh?

> +/* i.MX8Mq iomux gpr register field defines */
> +#define IMX8MQ_GPR13_MIPI_MUX_SEL              BIT(2)

I think this define should probably be local to the pinctrl driver, to
ensure that no other drivers fiddle with the registers manually.

     Arnd
Guido Günther Aug. 13, 2019, 10:10 a.m. UTC | #3 
Hi Arnd,
On Tue, Aug 13, 2019 at 10:08:44AM +0200, Arnd Bergmann wrote:
> On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
> >
> > This adds all the gpr registers and the define needed for selecting
> > the input source in the imx-nwl drm bridge.
> >
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > +
> > +#define IOMUXC_GPR0    0x00
> > +#define IOMUXC_GPR1    0x04
> > +#define IOMUXC_GPR2    0x08
> > +#define IOMUXC_GPR3    0x0c
> > +#define IOMUXC_GPR4    0x10
> > +#define IOMUXC_GPR5    0x14
> > +#define IOMUXC_GPR6    0x18
> > +#define IOMUXC_GPR7    0x1c
> (more of the same)
> 
> huh?

These are the names from the imx8MQ reference manual (general purpose
registers, they lump together all sorts of things), it's the same on
imx6/imx7):

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx7-iomuxc-gpr.h     

> > +/* i.MX8Mq iomux gpr register field defines */
> > +#define IMX8MQ_GPR13_MIPI_MUX_SEL              BIT(2)
> 
> I think this define should probably be local to the pinctrl driver, to
> ensure that no other drivers fiddle with the registers manually.

The purpose of these bits is for a driver to fiddle with them to select
the input source. Similar on imx7 it's already used for e.g. the phy
refclk in the pci controller:

    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c#n638

The GPRs are not about pad configuration but gather all sorts of things
(section 8.2.4 of the imx8mq reference manual): pcie setup, dsi related
bits so I don't think this should be done via a pinctrl
driver. Should we handle that differently than on imx6/7?

Cheers,
 -- Guido
Arnd Bergmann Aug. 13, 2019, 11:07 a.m. UTC | #4 
On Tue, Aug 13, 2019 at 12:10 PM Guido Günther <agx@sigxcpu.org> wrote:
> On Tue, Aug 13, 2019 at 10:08:44AM +0200, Arnd Bergmann wrote:
> > On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
> > >
> > > This adds all the gpr registers and the define needed for selecting
> > > the input source in the imx-nwl drm bridge.
> > >
> > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > +
> > > +#define IOMUXC_GPR0    0x00
> > > +#define IOMUXC_GPR1    0x04
> > > +#define IOMUXC_GPR2    0x08
> > > +#define IOMUXC_GPR3    0x0c
> > > +#define IOMUXC_GPR4    0x10
> > > +#define IOMUXC_GPR5    0x14
> > > +#define IOMUXC_GPR6    0x18
> > > +#define IOMUXC_GPR7    0x1c
> > (more of the same)
> >
> > huh?
>
> These are the names from the imx8MQ reference manual (general purpose
> registers, they lump together all sorts of things), it's the same on
> imx6/imx7):
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx7-iomuxc-gpr.h
>
> > > +/* i.MX8Mq iomux gpr register field defines */
> > > +#define IMX8MQ_GPR13_MIPI_MUX_SEL              BIT(2)
> >
> > I think this define should probably be local to the pinctrl driver, to
> > ensure that no other drivers fiddle with the registers manually.
>
> The purpose of these bits is for a driver to fiddle with them to select
> the input source. Similar on imx7 it's already used for e.g. the phy
> refclk in the pci controller:
>
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c#n638

That one should likely use either the clk interface or the phy
interface instead.

> The GPRs are not about pad configuration but gather all sorts of things
> (section 8.2.4 of the imx8mq reference manual): pcie setup, dsi related
> bits so I don't think this should be done via a pinctrl
> driver. Should we handle that differently than on imx6/7?

It would be nice to fix the existing code as well, but for the moment,
I only think we should not add more of that.

Generally speaking, we can use syscon to do random things that don't
have a subsystem of their own, but we should not use it to do things
that have an existing driver framework like pinctrl, clock, reset, phy
etc.

       Arnd
Guido Günther Aug. 21, 2019, 5:42 p.m. UTC | #5 
Hi,
On Tue, Aug 13, 2019 at 01:07:52PM +0200, Arnd Bergmann wrote:
> On Tue, Aug 13, 2019 at 12:10 PM Guido Günther <agx@sigxcpu.org> wrote:
> > On Tue, Aug 13, 2019 at 10:08:44AM +0200, Arnd Bergmann wrote:
> > > On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
> > > >
> > > > This adds all the gpr registers and the define needed for selecting
> > > > the input source in the imx-nwl drm bridge.
> > > >
> > > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > > +
> > > > +#define IOMUXC_GPR0    0x00
> > > > +#define IOMUXC_GPR1    0x04
> > > > +#define IOMUXC_GPR2    0x08
> > > > +#define IOMUXC_GPR3    0x0c
> > > > +#define IOMUXC_GPR4    0x10
> > > > +#define IOMUXC_GPR5    0x14
> > > > +#define IOMUXC_GPR6    0x18
> > > > +#define IOMUXC_GPR7    0x1c
> > > (more of the same)
> > >
> > > huh?
> >
> > These are the names from the imx8MQ reference manual (general purpose
> > registers, they lump together all sorts of things), it's the same on
> > imx6/imx7):
> >
> >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx7-iomuxc-gpr.h
> >
> > > > +/* i.MX8Mq iomux gpr register field defines */
> > > > +#define IMX8MQ_GPR13_MIPI_MUX_SEL              BIT(2)
> > >
> > > I think this define should probably be local to the pinctrl driver, to
> > > ensure that no other drivers fiddle with the registers manually.
> >
> > The purpose of these bits is for a driver to fiddle with them to select
> > the input source. Similar on imx7 it's already used for e.g. the phy
> > refclk in the pci controller:
> >
> >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c#n638
> 
> That one should likely use either the clk interface or the phy
> interface instead.
> 
> > The GPRs are not about pad configuration but gather all sorts of things
> > (section 8.2.4 of the imx8mq reference manual): pcie setup, dsi related
> > bits so I don't think this should be done via a pinctrl
> > driver. Should we handle that differently than on imx6/7?
> 
> It would be nice to fix the existing code as well, but for the moment,
> I only think we should not add more of that.
> 
> Generally speaking, we can use syscon to do random things that don't
> have a subsystem of their own, but we should not use it to do things
> that have an existing driver framework like pinctrl, clock, reset, phy
> etc.

Since it's not an external pin i opted to use MUX_MMIO instead which
seems like a good fit here. Does that make sense?
Cheers,
 -- Guido
Philipp Zabel Aug. 22, 2019, 10:03 a.m. UTC | #6 
On Wed, 2019-08-21 at 19:42 +0200, Guido Günther wrote:
> Hi,
> On Tue, Aug 13, 2019 at 01:07:52PM +0200, Arnd Bergmann wrote:
> > On Tue, Aug 13, 2019 at 12:10 PM Guido Günther <agx@sigxcpu.org> wrote:
> > > On Tue, Aug 13, 2019 at 10:08:44AM +0200, Arnd Bergmann wrote:
> > > > On Fri, Aug 9, 2019 at 6:24 PM Guido Günther <agx@sigxcpu.org> wrote:
> > > > > 
> > > > > This adds all the gpr registers and the define needed for selecting
> > > > > the input source in the imx-nwl drm bridge.
> > > > > 
> > > > > Signed-off-by: Guido Günther <agx@sigxcpu.org>
> > > > > +
> > > > > +#define IOMUXC_GPR0    0x00
> > > > > +#define IOMUXC_GPR1    0x04
> > > > > +#define IOMUXC_GPR2    0x08
> > > > > +#define IOMUXC_GPR3    0x0c
> > > > > +#define IOMUXC_GPR4    0x10
> > > > > +#define IOMUXC_GPR5    0x14
> > > > > +#define IOMUXC_GPR6    0x18
> > > > > +#define IOMUXC_GPR7    0x1c
> > > > 
> > > > (more of the same)
> > > > 
> > > > huh?
> > > 
> > > These are the names from the imx8MQ reference manual (general purpose
> > > registers, they lump together all sorts of things), it's the same on
> > > imx6/imx7):
> > > 
> > >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx6q-iomuxc-gpr.h
> > >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/mfd/syscon/imx7-iomuxc-gpr.h
> > > 
> > > > > +/* i.MX8Mq iomux gpr register field defines */
> > > > > +#define IMX8MQ_GPR13_MIPI_MUX_SEL              BIT(2)
> > > > 
> > > > I think this define should probably be local to the pinctrl driver, to
> > > > ensure that no other drivers fiddle with the registers manually.
> > > 
> > > The purpose of these bits is for a driver to fiddle with them to select
> > > the input source. Similar on imx7 it's already used for e.g. the phy
> > > refclk in the pci controller:
> > > 
> > >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pci-imx6.c#n638
> > 
> > That one should likely use either the clk interface or the phy
> > interface instead.
> > 
> > > The GPRs are not about pad configuration but gather all sorts of things
> > > (section 8.2.4 of the imx8mq reference manual): pcie setup, dsi related
> > > bits so I don't think this should be done via a pinctrl
> > > driver. Should we handle that differently than on imx6/7?
> > 
> > It would be nice to fix the existing code as well, but for the moment,
> > I only think we should not add more of that.
> > 
> > Generally speaking, we can use syscon to do random things that don't
> > have a subsystem of their own, but we should not use it to do things
> > that have an existing driver framework like pinctrl, clock, reset, phy
> > etc.
> 
> Since it's not an external pin i opted to use MUX_MMIO instead which
> seems like a good fit here. Does that make sense?

Yes, I agree. The i.MX6 display subsystem predates the mux framework,
otherwise I would have used it for the LVDS and HDMI muxes in the first
place. We should probably switch imx-drm over as well, in a backwards
compatible fashion. The &mux definitions are already there in
arch/arm/boot/dts/imx6q.dtsi.

regards
Philipp
diff mbox series

Patch

diff --git a/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h b/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h
new file mode 100644
index 000000000000..62e85ffacfad
--- /dev/null
+++ b/include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h
@@ -0,0 +1,62 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2017 NXP
+ *               2019 Purism SPC
+ */
+
+#ifndef __LINUX_IMX8MQ_IOMUXC_GPR_H
+#define __LINUX_IMX8MQ_IOMUXC_GPR_H
+
+#define IOMUXC_GPR0	0x00
+#define IOMUXC_GPR1	0x04
+#define IOMUXC_GPR2	0x08
+#define IOMUXC_GPR3	0x0c
+#define IOMUXC_GPR4	0x10
+#define IOMUXC_GPR5	0x14
+#define IOMUXC_GPR6	0x18
+#define IOMUXC_GPR7	0x1c
+#define IOMUXC_GPR8	0x20
+#define IOMUXC_GPR9	0x24
+#define IOMUXC_GPR10	0x28
+#define IOMUXC_GPR11	0x2c
+#define IOMUXC_GPR12	0x30
+#define IOMUXC_GPR13	0x34
+#define IOMUXC_GPR14	0x38
+#define IOMUXC_GPR15	0x3c
+#define IOMUXC_GPR16	0x40
+#define IOMUXC_GPR17	0x44
+#define IOMUXC_GPR18	0x48
+#define IOMUXC_GPR19	0x4c
+#define IOMUXC_GPR20	0x50
+#define IOMUXC_GPR21	0x54
+#define IOMUXC_GPR22	0x58
+#define IOMUXC_GPR23	0x5c
+#define IOMUXC_GPR24	0x60
+#define IOMUXC_GPR25	0x64
+#define IOMUXC_GPR26	0x68
+#define IOMUXC_GPR27	0x6c
+#define IOMUXC_GPR28	0x70
+#define IOMUXC_GPR29	0x74
+#define IOMUXC_GPR30	0x78
+#define IOMUXC_GPR31	0x7c
+#define IOMUXC_GPR32	0x80
+#define IOMUXC_GPR33	0x84
+#define IOMUXC_GPR34	0x88
+#define IOMUXC_GPR35	0x8c
+#define IOMUXC_GPR36	0x90
+#define IOMUXC_GPR37	0x94
+#define IOMUXC_GPR38	0x98
+#define IOMUXC_GPR39	0x9c
+#define IOMUXC_GPR40	0xa0
+#define IOMUXC_GPR41	0xa4
+#define IOMUXC_GPR42	0xa8
+#define IOMUXC_GPR43	0xac
+#define IOMUXC_GPR44	0xb0
+#define IOMUXC_GPR45	0xb4
+#define IOMUXC_GPR46	0xb8
+#define IOMUXC_GPR47	0xbc
+
+/* i.MX8Mq iomux gpr register field defines */
+#define IMX8MQ_GPR13_MIPI_MUX_SEL		BIT(2)
+
+#endif /* __LINUX_IMX8MQ_IOMUXC_GPR_H */