| 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 |
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 */
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
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
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
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
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 --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 */
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