| Message ID | 20180102164059.14641-1-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 2, 2018 at 5:40 PM, Stefan Agner <stefan@agner.ch> wrote: > From: Bai Ping <ping.bai@nxp.com> > > On i.MX 6ULL, the BOOT_MODEx and TAMPERx pin MUX and CTRL registers > are available in a separate IOMUXC_SNVS module. Add support for the > IOMUXC_SNVS module to the i.MX 6UL pinctrl driver. > > Signed-off-by: Bai Ping <ping.bai@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> So 6 unsigned long 32 bit is succeeded by 6 unsigned long long, 64 bit? Someone is having fun naming these platforms I see. > Required properties: > -- compatible: "fsl,imx6ul-iomuxc" > +- compatible: "fsl,imx6ul-iomuxc" for main IOMUX controller or > + "fsl,imx6ull-iomuxc-snvs" for i.MX 6ULL's SNVS IOMUX controller. Pretty uncontroversial change but still nice to give the DT people a chance to ACK it. > static int imx6ul_pinctrl_probe(struct platform_device *pdev) > { > - return imx_pinctrl_probe(pdev, &imx6ul_pinctrl_info); > + const struct of_device_id *match; > + struct imx_pinctrl_soc_info *pinctrl_info; > + > + match = of_match_device(imx6ul_pinctrl_of_match, &pdev->dev); > + > + if (!match) > + return -ENODEV; > + > + pinctrl_info = (struct imx_pinctrl_soc_info *) match->data; 1. Do not use a cast on void * pointers. 2. Use this function: extern const void *of_device_get_match_data(const struct device *dev); From <linux/of_device.h> Yours, Linus Walleij
On Tue, Jan 02, 2018 at 05:40:59PM +0100, Stefan Agner wrote: > From: Bai Ping <ping.bai@nxp.com> > > On i.MX 6ULL, the BOOT_MODEx and TAMPERx pin MUX and CTRL registers > are available in a separate IOMUXC_SNVS module. Add support for the > IOMUXC_SNVS module to the i.MX 6UL pinctrl driver. > > Signed-off-by: Bai Ping <ping.bai@nxp.com> > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > .../bindings/pinctrl/fsl,imx6ul-pinctrl.txt | 3 +- Reviewed-by: Rob Herring <robh@kernel.org> > drivers/pinctrl/freescale/pinctrl-imx6ul.c | 52 +++++++++++++++++++++- > 2 files changed, 52 insertions(+), 3 deletions(-)
On 2018-01-03 09:09, Linus Walleij wrote: > On Tue, Jan 2, 2018 at 5:40 PM, Stefan Agner <stefan@agner.ch> wrote: > >> From: Bai Ping <ping.bai@nxp.com> >> >> On i.MX 6ULL, the BOOT_MODEx and TAMPERx pin MUX and CTRL registers >> are available in a separate IOMUXC_SNVS module. Add support for the >> IOMUXC_SNVS module to the i.MX 6UL pinctrl driver. >> >> Signed-off-by: Bai Ping <ping.bai@nxp.com> >> Signed-off-by: Stefan Agner <stefan@agner.ch> > > So 6 unsigned long 32 bit is succeeded by 6 unsigned long long, 64 bit? > > Someone is having fun naming these platforms I see. Hehe, yeah definitely. At least they use proper type suffixes ;-) > >> Required properties: >> -- compatible: "fsl,imx6ul-iomuxc" >> +- compatible: "fsl,imx6ul-iomuxc" for main IOMUX controller or >> + "fsl,imx6ull-iomuxc-snvs" for i.MX 6ULL's SNVS IOMUX controller. > > Pretty uncontroversial change but still nice to give the DT people a chance > to ACK it. > >> static int imx6ul_pinctrl_probe(struct platform_device *pdev) >> { >> - return imx_pinctrl_probe(pdev, &imx6ul_pinctrl_info); >> + const struct of_device_id *match; >> + struct imx_pinctrl_soc_info *pinctrl_info; >> + >> + match = of_match_device(imx6ul_pinctrl_of_match, &pdev->dev); >> + >> + if (!match) >> + return -ENODEV; >> + >> + pinctrl_info = (struct imx_pinctrl_soc_info *) match->data; > > 1. Do not use a cast on void * pointers. > > 2. Use this function: > extern const void *of_device_get_match_data(const struct device *dev); > > From <linux/of_device.h> Ok. We use the same code in pinmux-imx7d.c, will send a patch to simplify that too. -- Stefan
On 2018-01-06 12:14, Stefan Agner wrote: > On 2018-01-03 09:09, Linus Walleij wrote: >> On Tue, Jan 2, 2018 at 5:40 PM, Stefan Agner <stefan@agner.ch> wrote: >> >>> From: Bai Ping <ping.bai@nxp.com> >>> >>> On i.MX 6ULL, the BOOT_MODEx and TAMPERx pin MUX and CTRL registers >>> are available in a separate IOMUXC_SNVS module. Add support for the >>> IOMUXC_SNVS module to the i.MX 6UL pinctrl driver. >>> >>> Signed-off-by: Bai Ping <ping.bai@nxp.com> >>> Signed-off-by: Stefan Agner <stefan@agner.ch> >> >> So 6 unsigned long 32 bit is succeeded by 6 unsigned long long, 64 bit? >> >> Someone is having fun naming these platforms I see. > > Hehe, yeah definitely. At least they use proper type suffixes ;-) > >> >>> Required properties: >>> -- compatible: "fsl,imx6ul-iomuxc" >>> +- compatible: "fsl,imx6ul-iomuxc" for main IOMUX controller or >>> + "fsl,imx6ull-iomuxc-snvs" for i.MX 6ULL's SNVS IOMUX controller. >> >> Pretty uncontroversial change but still nice to give the DT people a chance >> to ACK it. >> >>> static int imx6ul_pinctrl_probe(struct platform_device *pdev) >>> { >>> - return imx_pinctrl_probe(pdev, &imx6ul_pinctrl_info); >>> + const struct of_device_id *match; >>> + struct imx_pinctrl_soc_info *pinctrl_info; >>> + >>> + match = of_match_device(imx6ul_pinctrl_of_match, &pdev->dev); >>> + >>> + if (!match) >>> + return -ENODEV; >>> + >>> + pinctrl_info = (struct imx_pinctrl_soc_info *) match->data; >> >> 1. Do not use a cast on void * pointers. >> >> 2. Use this function: >> extern const void *of_device_get_match_data(const struct device *dev); Just realized that imx_pinctrl_probe needs a non-const struct imx_pinctrl_soc_info *... Afaik, while casting to non-const struct pointers in imxXX_pinctrl_probe is not nice but should be technically ok since it is a non-const declaration. However, in pinmux-imx7d.c the struct imx_pinctrl_soc_info declarations have even been constified with b3060044e495 ("pinctrl: freescale: imx7d: make of_device_ids const"). That seems not ok! I wonder how that even works under the light of CONFIG_STRICT_KERNEL_RWX. In a quick test it seems to boot fine though, as far as I can tell rodata are set to ro once boot completed... It seems at that point the pinmux driver has been initialized completely and does not write to the struct anymore. How can we fix this properly? IMHO we should at least unconstify the struct imx7d_pinctrl_info/imx7d_lpsr_pinctrl_info declarations in pinctrl-imx7d.c... -- Stefan >> >> From <linux/of_device.h> > > Ok. We use the same code in pinmux-imx7d.c, will send a patch to > simplify that too. > > -- > Stefan
diff --git a/Documentation/devicetree/bindings/pinctrl/fsl,imx6ul-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fsl,imx6ul-pinctrl.txt index a81bbf37ed66..7ca4f6118d9a 100644 --- a/Documentation/devicetree/bindings/pinctrl/fsl,imx6ul-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/fsl,imx6ul-pinctrl.txt @@ -4,7 +4,8 @@ Please refer to fsl,imx-pinctrl.txt in this directory for common binding part and usage. Required properties: -- compatible: "fsl,imx6ul-iomuxc" +- compatible: "fsl,imx6ul-iomuxc" for main IOMUX controller or + "fsl,imx6ull-iomuxc-snvs" for i.MX 6ULL's SNVS IOMUX controller. - fsl,pins: each entry consists of 6 integers and represents the mux and config setting for one pin. The first 5 integers <mux_reg conf_reg input_reg mux_val input_val> are specified using a PIN_FUNC_ID macro, which can be found in diff --git a/drivers/pinctrl/freescale/pinctrl-imx6ul.c b/drivers/pinctrl/freescale/pinctrl-imx6ul.c index 1aeb840aae1d..cbad1c69226d 100644 --- a/drivers/pinctrl/freescale/pinctrl-imx6ul.c +++ b/drivers/pinctrl/freescale/pinctrl-imx6ul.c @@ -150,6 +150,21 @@ enum imx6ul_pads { MX6UL_PAD_CSI_DATA07 = 128, }; +enum imx6ull_lpsr_pads { + MX6ULL_PAD_BOOT_MODE0 = 0, + MX6ULL_PAD_BOOT_MODE1 = 1, + MX6ULL_PAD_SNVS_TAMPER0 = 2, + MX6ULL_PAD_SNVS_TAMPER1 = 3, + MX6ULL_PAD_SNVS_TAMPER2 = 4, + MX6ULL_PAD_SNVS_TAMPER3 = 5, + MX6ULL_PAD_SNVS_TAMPER4 = 6, + MX6ULL_PAD_SNVS_TAMPER5 = 7, + MX6ULL_PAD_SNVS_TAMPER6 = 8, + MX6ULL_PAD_SNVS_TAMPER7 = 9, + MX6ULL_PAD_SNVS_TAMPER8 = 10, + MX6ULL_PAD_SNVS_TAMPER9 = 11, +}; + /* Pad names for the pinmux subsystem */ static const struct pinctrl_pin_desc imx6ul_pinctrl_pads[] = { IMX_PINCTRL_PIN(MX6UL_PAD_RESERVE0), @@ -283,20 +298,53 @@ static const struct pinctrl_pin_desc imx6ul_pinctrl_pads[] = { IMX_PINCTRL_PIN(MX6UL_PAD_CSI_DATA07), }; +/* pad for i.MX6ULL lpsr pinmux */ +static struct pinctrl_pin_desc imx6ull_snvs_pinctrl_pads[] = { + IMX_PINCTRL_PIN(MX6ULL_PAD_BOOT_MODE0), + IMX_PINCTRL_PIN(MX6ULL_PAD_BOOT_MODE1), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER0), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER1), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER2), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER3), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER4), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER5), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER6), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER7), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER8), + IMX_PINCTRL_PIN(MX6ULL_PAD_SNVS_TAMPER9), +}; + static struct imx_pinctrl_soc_info imx6ul_pinctrl_info = { .pins = imx6ul_pinctrl_pads, .npins = ARRAY_SIZE(imx6ul_pinctrl_pads), .gpr_compatible = "fsl,imx6ul-iomuxc-gpr", }; +static struct imx_pinctrl_soc_info imx6ull_snvs_pinctrl_info = { + .pins = imx6ull_snvs_pinctrl_pads, + .npins = ARRAY_SIZE(imx6ull_snvs_pinctrl_pads), + .flags = ZERO_OFFSET_VALID, +}; + static struct of_device_id imx6ul_pinctrl_of_match[] = { - { .compatible = "fsl,imx6ul-iomuxc", }, + { .compatible = "fsl,imx6ul-iomuxc", .data = &imx6ul_pinctrl_info, }, + { .compatible = "fsl,imx6ull-iomuxc-snvs", .data = &imx6ull_snvs_pinctrl_info, }, { /* sentinel */ } }; static int imx6ul_pinctrl_probe(struct platform_device *pdev) { - return imx_pinctrl_probe(pdev, &imx6ul_pinctrl_info); + const struct of_device_id *match; + struct imx_pinctrl_soc_info *pinctrl_info; + + match = of_match_device(imx6ul_pinctrl_of_match, &pdev->dev); + + if (!match) + return -ENODEV; + + pinctrl_info = (struct imx_pinctrl_soc_info *) match->data; + + return imx_pinctrl_probe(pdev, pinctrl_info); } static struct platform_driver imx6ul_pinctrl_driver = {