pinctrl: pinctrl-imx: add support for set bits for general purpose registers

From: Dong Aisheng <[email protected]>

The original pin registers table is derived from u-boot mainline, but somehow it was found missing some mux functions for USBOTG_ID. We added it at the bottom by following the exist pin function ids, then it will not break the exist using of pin function id in dts file.

Reported-by Richard Zhao <[email protected]>

Signed-off-by Dong Aisheng <[email protected]>

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

Quoted 7 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 66

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +10 -0

+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
+{
+	u32 reg;
+
+	/* general purpose register is 32 bits size */
+	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);

Hmmm. It's going to be very hard to control the probe() order to ensure that this WARN doesn't fire all the time.

I think it would be better to pass in a struct imx_pinctrl* or DT node to this function. The DT node for the device that's using this function should contain a phandle to the pinctrl device node, which it uses to get that handle. Or in a non-DT case, the client driver needs to be given the provider driver handle using some other mechanism.

For example, look at how the Tegra30 SMMU uses services from the Tegra30 AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with this handle) and drivers/amba/tegra-ahb.c function tegra_ahb_enable_smmu() (implements the deferred probe checking to correctly order the client/provider driver probing)

Quoted 9 lines · Stephen Warren

Quoted 7 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

Quoted 14 lines

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 66

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +10 -0

+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
+{
+	u32 reg;
+
+	/* general purpose register is 32 bits size */
+	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);

Hmmm. It's going to be very hard to control the probe() order to ensure that this WARN doesn't fire all the time.

Correct, if this api is used by client driver before the pinctrl driver is registered, the warning will show.
To avoid it, the current pinctrl driver register priority is arch_initcall.

But maybe you're right, this may not be so sufficient since i'm afraid there are still possible some devices may register earlier than pinctrl since it's not controlled by pinctrl driver.
Then it's true that we need to resolve it.

Quoted 3 lines

I think it would be better to pass in a struct imx_pinctrl* or DT node to this function.

The DT node for the device that's using this function

Quoted 4 lines

should contain a phandle to the pinctrl device node, which it uses to get that handle. Or in a non-DT case, the client driver needs to be given the provider driver handle using some other mechanism.

Quoted 8 lines

For example, look at how the Tegra30 SMMU uses services from the Tegra30 AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with this handle) and drivers/amba/tegra-ahb.c function tegra_ahb_enable_smmu() (implements the deferred probe checking to correctly order the client/provider driver probing)

Yes, i learned the code but i'm not sure it also fits for imx.
I have a few concerns:
1) i'm not sure if we really need the client to provide pinctrl device node as parameter to set gpr registers since there is only one pinctrl device for each imx soc. Client devices may also not want to care that parameter.
2) if passing device node to the api, that means every client driver needs to define a phandle of pinctrl device in dts file and parsing it in each driver respectively.
There're several client devices for imx6q, i'm not sure if it's worth to do that considering this overhead.

I think either passing device node or not passing, the goal is the same, guarantee this api to be used properly without being affected by driver loading sequence.

If that, how about change to:
int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)

{
        u32 reg;

	if (!imx_pinctrl)
		return -EDEFER_PROBE;

        /* general purpose register is 32 bits size */
        WARN_ON(start_bit > 31 || num_bits > 32);

...
}
EXPORT_SYMBOL_GPL(imx_pinctrl_set_gpr_register);

Then it looks to me it may be able to solve the same issue.

Regards
Dong Aisheng

Quoted 24 lines · Dong Aisheng

Quoted 23 lines · Stephen Warren

Quoted 8 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

drivers/pinctrl/pinctrl-imx.c struct imx_pinctrl { · line 66

Quoted 10 lines

drivers/pinctrl/pinctrl-imx.c +10 -0

+static struct imx_pinctrl *imx_pinctrl;
+/*
+ * Set bits for general purpose registers
+ */
+void imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)
+{
+	u32 reg;
+
+	/* general purpose register is 32 bits size */
+	WARN_ON(!imx_pinctrl || start_bit > 31 || num_bits > 32);

Hmmm. It's going to be very hard to control the probe() order to ensure that this WARN doesn't fire all the time.

Quoted 3 lines

Correct, if this api is used by client driver before the pinctrl driver is registered, the warning will show.
To avoid it, the current pinctrl driver register priority is arch_initcall.

+postcore_initcall is not enough. Could you use postcore_initcall? So people can hack right after populate devices.

Quoted 4 lines

But maybe you're right, this may not be so sufficient since i'm afraid there are still possible some devices may register earlier than pinctrl since it's not controlled by pinctrl driver.
Then it's true that we need to resolve it.

imx_pinctrl_set_gpr_register is SoC specific. People who use it must have sense of related registers and driver loading sequence.

Quoted 3 lines

Quoted 3 lines

I think it would be better to pass in a struct imx_pinctrl* or DT node to this function.

Maybe we can do it, but why? It's kind of over-engineering.

Quoted 33 lines

The DT node for the device that's using this function

Quoted 4 lines

should contain a phandle to the pinctrl device node, which it uses to get that handle. Or in a non-DT case, the client driver needs to be given the provider driver handle using some other mechanism.

Quoted 8 lines

For example, look at how the Tegra30 SMMU uses services from the Tegra30 AHB; see arch/arm/boot/dts/tegra30.dtsi node "smmu" (client) and node "ahb" (provider), drivers/iommu/tegra-smmu.c functions probe() (saves smmu->ahb) and smmu_setup_regs() (calls tegra_ahb_enable_smmu() with this handle) and drivers/amba/tegra-ahb.c function tegra_ahb_enable_smmu() (implements the deferred probe checking to correctly order the client/provider driver probing)

Yes, i learned the code but i'm not sure it also fits for imx.
I have a few concerns:
1) i'm not sure if we really need the client to provide pinctrl device node as parameter to set gpr registers since there is only one pinctrl device for each imx soc. Client devices may also not want to care that parameter.
2) if passing device node to the api, that means every client driver needs to define a phandle of pinctrl device in dts file and parsing it in each driver respectively.
There're several client devices for imx6q, i'm not sure if it's worth to do that considering this overhead.

I think either passing device node or not passing, the goal is the same, guarantee this api to be used properly without being affected by driver loading sequence.

If that, how about change to:
int imx_pinctrl_set_gpr_register(u8 gpr, u8 start_bit, u8 num_bits, u32 value)

{
        u32 reg;

Quoted 5 lines

	if (!imx_pinctrl)
		return -EDEFER_PROBE;

        /* general purpose register is 32 bits size */
        WARN_ON(start_bit > 31 || num_bits > 32);

Isn't it BUG ?

Normally, people like write_register(addr, mask, value), and read_register(addr, mask);

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

Signed-off-by Dong Aisheng <[email protected]>

Quoted 8 lines · Dong Aisheng

From: Dong Aisheng <[email protected]>

The General Purpose Registers (GPR) is used to select operating modes for general features in the SoC, usually not related to the IOMUX itself, but it does belong to IOMUX controller.
We simply provide an convient API for driver to call to set the general purpose register bits if needed.

Quoted 1 line

Signed-off-by Dong Aisheng <[email protected]>

I have an overall objection to this:

I have no idea at all about what's going on, and why this belongs in the pin control subsystem. On the contrary, from the commit description it seems to *not* belong in this subsystem at all.

So: exactly what does this register do, and which are the consumers?