| Message ID | cover.1565367567.git.agx@sigxcpu.org |
|---|---|
| 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
This adds initial support for the NWL MIPI DSI Host controller found on i.MX8 SoCs. It adds support for the i.MX8MQ but the same IP core can also be found on e.g. i.MX8QXP. I added the necessary hooks to support other imx8 variants but since I only have imx8mq boards to test I omitted the platform data for other SoCs. The code is based on NXPs BSP so I added Robert Chiras as Co-authored-by. Robert, if this looks sane could you add your Signed-off-by:? The most notable changes over the BSP driver are - Calculate HS mode timing from phy_configure_opts_mipi_dphy - Perform all clock setup via DT - Merge nwl-imx and nwl drivers - Add B0 silion revision quirk - Become a bridge driver to hook into mxsfb (from what I read[0] DCSS, which also can drive the nwl on the imx8mq will likely not become part of imx-display-subsystem so it makes sense to make it drive a bridge for dsi as well). - Use panel_bridge to attach the panel This has been tested on a Librem 5 devkit using mxsfb with Robert's patches[1] and the rocktech-jh057n00900 panel driver on next-20190807. The DCSS can later on also act as input source too. Changes from v1: - Per review comments by Sam Ravnborg https://lists.freedesktop.org/archives/dri-devel/2019-July/228130.html - Change binding docs to YAML - build: Don't always visit imx-nwl/ - build: Add header-test-y - Sort headers according to DRM convention - Use drm_display_mode instead of videmode - Per review comments by Fabio Estevam https://lists.freedesktop.org/archives/dri-devel/2019-July/228299.html - Don't restrict build to ARCH_MXC - Drop unused includes - Drop unreachable code in imx_nwl_dsi_bridge_mode_fixup() - Drop remaining calls of dev_err() and use DRM_DEV_ERR() consistently. - Use devm_platform_ioremap_resource() - Drop devm_free_irq() in probe() error path - Use single line comments where sufficient - Use <linux/time64.h> instead of defining USEC_PER_SEC - Make input source select imx8 specific - Drop <asm/unaligned.h> inclusion (after removal of get_unaligned_le32) - Drop all EXPORT_SYMBOL_GPL() for functions used in the same module but different source files. - Drop nwl_dsi_enable_{rx,tx}_clock() by invoking clk_prepare_enable() directly - Remove pointless comment - Laurent Pinchart https://lists.freedesktop.org/archives/dri-devel/2019-July/228313.html https://lists.freedesktop.org/archives/dri-devel/2019-July/228308.html - Drop (on iMX8MQ) unused csr regmap - Use NWL_MAX_PLATFORM_CLOCKS everywhere - Drop get_unaligned_le32() usage - remove duplicate 'for the' in binding docs - Don't include unused <linux/clk-provider.h> - Don't include unused <linux/component.h> - Drop dpms_mode for tracking state, trust the drm layer on that - Use pm_runtime_put() instead of pm_runtime_put_sync() - Don't overwrite encoder type - Make imx_nwl_platform_data const - Use the reset controller API instead of open coding that platform specific part - Use <linux/bitfield.h> intead of making up our own defines - name mipi_dsi_transfer less generic: nwl_dsi_transfer - ensure clean in .remove by calling mipi_dsi_host_unregister. - prefix constants by NWL_DSI_ - properly format transfer_direction enum - simplify platform clock handling - Don't modify state in mode_fixup() and use mode_set() instead - Drop bridge detach(), already handle by nwl_dsi_host_detach() - Drop USE_*_QUIRK() macros - Drop (for now) unused clock defnitions. 'pixel' and 'bypass' clock will be used for i.MX8 SoCs but since they're unused atm drop the definitions - but keep the logic to enable/disable several clocks in place since we know we'll need it in the future. Changes from v0: - Add quirk for IMQ8MQ silicon B0 revision to not mess with the system reset controller on power down since enable() won't work otherwise. - Drop devm_free_irq() handled by the device driver core - Disable tx esc clock after the phy power down to unbreak disable/enable (unblank/blank) - Add ports to dt binding docs - Select GENERIC_PHY_MIPI_DPHY instead of GENERIC_PHY for phy_mipi_dphy_get_default_config - Select DRM_MIPI_DSI - Include drm_print.h to fix build on next-20190408 - Drop some debugging messages - Newline terminate all DRM_ printouts - Turn component driver into a drm bridge [0]: https://lists.freedesktop.org/archives/dri-devel/2019-May/219484.html [1]: https://patchwork.freedesktop.org/series/62822/ Guido Günther (3): arm64: imx8mq: add imx8mq iomux-gpr field defines dt-bindings: display/bridge: Add binding for NWL mipi dsi host controller drm/bridge: Add NWL MIPI DSI host controller support .../bindings/display/bridge/nwl-dsi.yaml | 155 ++++ drivers/gpu/drm/bridge/Kconfig | 2 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/nwl-dsi/Kconfig | 15 + drivers/gpu/drm/bridge/nwl-dsi/Makefile | 4 + drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c | 484 ++++++++++++ drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h | 66 ++ drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c | 700 ++++++++++++++++++ drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.h | 112 +++ include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h | 62 ++ 10 files changed, 1601 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/Kconfig create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/Makefile create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.h create mode 100644 include/linux/mfd/syscon/imx8mq-iomuxc-gpr.h