[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[Stefan Agner]

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 +-
 drivers/pinctrl/freescale/pinctrl-imx6ul.c         | 52 +++++++++++++++++++++-
 2 files changed, 52 insertions(+), 3 deletions(-)

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 = {
-- 
2.15.1

[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[Linus Walleij]

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

[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[Rob Herring]

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(-)

[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[Stefan Agner]

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

[PATCH] pinctrl: imx6ul: add IOMUXC SNVS pinctrl driver for i.MX 6ULL

[Stefan Agner]

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