diff mbox series

[v2,6/6] soc: ti: pruss: Add helper functions to get/set PRUSS_CFG_GPMUX

Message ID 20220418123004.9332-7-p-mohan@ti.com (mailing list archive)
State Not Applicable
Headers show
Series Introduce PRU platform consumer API | expand

Commit Message

Puranjay Mohan April 18, 2022, 12:30 p.m. UTC 
From: Tero Kristo <t-kristo@ti.com>

Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
to get and set the GP MUX mode for programming the PRUSS internal wrapper
mux functionality as needed by usecases.

Co-developed-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
---
 include/linux/pruss_driver.h | 44 ++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Romain Naour Sept. 12, 2022, 2:20 p.m. UTC | #1 
+Danish

Hi Danish,

(Removed Puranjay (as he is no longer with TI) and adding Danish.)

Le 18/04/2022 à 14:30, Puranjay Mohan a écrit :
> From: Tero Kristo <t-kristo@ti.com>
> 
> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
> to get and set the GP MUX mode for programming the PRUSS internal wrapper
> mux functionality as needed by usecases.

Actually I'm curious about how the GP MUX mode are supposed to work in some
cases. The register mapping in the AM57xx TRM seems confusing.

See the "PRU-ICSS I/O Interface" part about the "PRU-ICSS Internal Wrapper
Multiplexing" [1].

The commit "ARM: dts: am57xx-idk: Add prueth on ICSS" [2] (only in the
TI kernel tree) adds pruss1 and pruss2 for the am571x-idk board.

But this commit doesn't really explain the ti,pruss-gp-mux-sel setting
from pruss1_eth and pruss2_eth:

    /* Dual mac ethernet application node on icss1 */
    pruss1_eth {
    	status = "okay";
    	compatible = "ti,am57-prueth";

    	ti,pruss-gp-mux-sel = <0>,	/* GP, default */
    			      <4>;	/* MII2, needed for PRUSS1_MII1 */
    }

    &pruss2_eth {
    	ti,pruss-gp-mux-sel = <4>,	/* MII2, needed for PRUSS1_MII0 */
    			      <4>;	/* MII2, needed for PRUSS1_MII1 */
    };

At the first look, the two comments in pruss2_eth node about PRUSS1_MIIx seems
dubious. Indeed, it would means that the PRUSS2 setting (ti,pruss-gp-mux-sel) is
required to makes PRUSS1 work.

In my use case, only the pruss1 is expected to be used with the prueth driver.

Actually, the prueth on PRUSS1 partially works with only pruss1_eth's gp-mux
initialized:

    pruss1_eth {
            status = "okay";
            compatible = "ti,am57-prueth";

            ti,pruss-gp-mux-sel = <0>,      /* GP, default */
                                  <4>;      /* MII2, needed for PRUSS1_MII1 */
    }

    pruss2_eth {
            status = "disabled";
    }

(Tests done with the ti-linux-kernel 5.10.y)

On wireshark I noticed ethernet frames (ping) sent from the board but the reply
from the remote PC is never received on the board.

It really seems we need pruss2_eth's gp-mux initialized.
The problem here is that I don't want to enable PRUSS2 just to
configure pruss2_eth's gp-mux for the sake of pruss1.

I had to write manually (using devmem2) the "good" value (0x10002003) in
PRUSS2_CFG0 and PRUSS2_CFG1 to configure entirely the PRUSS1_MII1.

I'm not sure how the driver should handle this register mapping properly.

[1] https://www.ti.com/lit/ds/symlink/am5749.pdf

[2]
https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=linux-5.10.y&id=2a3b089f5697fe2f9a9875b2fba1bef88d196a53

Best regards,
Romain

> 
> Co-developed-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Tero Kristo <t-kristo@ti.com>
> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
> ---
>  include/linux/pruss_driver.h | 44 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
> index e2d5477225c6..3312281ef4c1 100644
> --- a/include/linux/pruss_driver.h
> +++ b/include/linux/pruss_driver.h
> @@ -35,4 +35,48 @@ struct pruss {
>  	struct clk *iep_clk_mux;
>  };
>  
> +/**
> + * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
> + * @pruss: pruss instance
> + * @pru_id: PRU identifier (0-1)
> + * @mux: pointer to store the current mux value into
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +static inline int pruss_cfg_get_gpmux(struct pruss *pruss,
> +				      enum pruss_pru_id pru_id, u8 *mux)
> +{
> +	int ret = 0;
> +	u32 val;
> +
> +	if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
> +		return -EINVAL;
> +
> +	ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
> +	if (!ret)
> +		*mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
> +			    PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
> +	return ret;
> +}
> +
> +/**
> + * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
> + * @pruss: pruss instance
> + * @pru_id: PRU identifier (0-1)
> + * @mux: new mux value for PRU
> + *
> + * Return: 0 on success, or an error code otherwise
> + */
> +static inline int pruss_cfg_set_gpmux(struct pruss *pruss,
> +				      enum pruss_pru_id pru_id, u8 mux)
> +{
> +	if (mux >= PRUSS_GP_MUX_SEL_MAX ||
> +	    pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
> +		return -EINVAL;
> +
> +	return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
> +				PRUSS_GPCFG_PRU_MUX_SEL_MASK,
> +				(u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
> +}
> +
>  #endif	/* _PRUSS_DRIVER_H_ */
Roger Quadros Sept. 14, 2022, 1:15 p.m. UTC | #2 
Hello Romain,

On 12/09/2022 17:20, Romain Naour wrote:
> +Danish
> 
> Hi Danish,
> 
> (Removed Puranjay (as he is no longer with TI) and adding Danish.)
> 
> Le 18/04/2022 à 14:30, Puranjay Mohan a écrit :
>> From: Tero Kristo <t-kristo@ti.com>
>>
>> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
>> to get and set the GP MUX mode for programming the PRUSS internal wrapper
>> mux functionality as needed by usecases.
> 
> Actually I'm curious about how the GP MUX mode are supposed to work in some
> cases. The register mapping in the AM57xx TRM seems confusing.
> 
> See the "PRU-ICSS I/O Interface" part about the "PRU-ICSS Internal Wrapper
> Multiplexing" [1].
> 
> The commit "ARM: dts: am57xx-idk: Add prueth on ICSS" [2] (only in the
> TI kernel tree) adds pruss1 and pruss2 for the am571x-idk board.
> 
> But this commit doesn't really explain the ti,pruss-gp-mux-sel setting
> from pruss1_eth and pruss2_eth:
> 
>     /* Dual mac ethernet application node on icss1 */
>     pruss1_eth {
>     	status = "okay";
>     	compatible = "ti,am57-prueth";
> 
>     	ti,pruss-gp-mux-sel = <0>,	/* GP, default */
>     			      <4>;	/* MII2, needed for PRUSS1_MII1 */
>     }
> 
>     &pruss2_eth {
>     	ti,pruss-gp-mux-sel = <4>,	/* MII2, needed for PRUSS1_MII0 */
>     			      <4>;	/* MII2, needed for PRUSS1_MII1 */
>     };
> 
> At the first look, the two comments in pruss2_eth node about PRUSS1_MIIx seems
> dubious. Indeed, it would means that the PRUSS2 setting (ti,pruss-gp-mux-sel) is
> required to makes PRUSS1 work.

Yes, if I remember right this is only applicable to AM571 Soc which had
an overloaded pinmuxing design and it resulted in this kind of weird constraint.
i.e. gp-mux-sel of PRUSS2 needs to be set to get PRUSS1 MII to work.

> 
> In my use case, only the pruss1 is expected to be used with the prueth driver.
> 
> Actually, the prueth on PRUSS1 partially works with only pruss1_eth's gp-mux
> initialized:
> 
>     pruss1_eth {
>             status = "okay";
>             compatible = "ti,am57-prueth";
> 
>             ti,pruss-gp-mux-sel = <0>,      /* GP, default */
>                                   <4>;      /* MII2, needed for PRUSS1_MII1 */
>     }
> 
>     pruss2_eth {
>             status = "disabled";
>     }
> 
> (Tests done with the ti-linux-kernel 5.10.y)
> 
> On wireshark I noticed ethernet frames (ping) sent from the board but the reply
> from the remote PC is never received on the board.
> 
> It really seems we need pruss2_eth's gp-mux initialized.
> The problem here is that I don't want to enable PRUSS2 just to
> configure pruss2_eth's gp-mux for the sake of pruss1.
> 
> I had to write manually (using devmem2) the "good" value (0x10002003) in
> PRUSS2_CFG0 and PRUSS2_CFG1 to configure entirely the PRUSS1_MII1.
> 
> I'm not sure how the driver should handle this register mapping properly.
> 
> [1] https://www.ti.com/lit/ds/symlink/am5749.pdf
> 
> [2]
> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=linux-5.10.y&id=2a3b089f5697fe2f9a9875b2fba1bef88d196a53
> 
> Best regards,
> Romain

cheers,
-roger

> 
>>
>> Co-developed-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Suman Anna <s-anna@ti.com>
>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>> ---
>>  include/linux/pruss_driver.h | 44 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 44 insertions(+)
>>
>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
>> index e2d5477225c6..3312281ef4c1 100644
>> --- a/include/linux/pruss_driver.h
>> +++ b/include/linux/pruss_driver.h
>> @@ -35,4 +35,48 @@ struct pruss {
>>  	struct clk *iep_clk_mux;
>>  };
>>  
>> +/**
>> + * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
>> + * @pruss: pruss instance
>> + * @pru_id: PRU identifier (0-1)
>> + * @mux: pointer to store the current mux value into
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +static inline int pruss_cfg_get_gpmux(struct pruss *pruss,
>> +				      enum pruss_pru_id pru_id, u8 *mux)
>> +{
>> +	int ret = 0;
>> +	u32 val;
>> +
>> +	if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>> +		return -EINVAL;
>> +
>> +	ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
>> +	if (!ret)
>> +		*mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
>> +			    PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
>> +	return ret;
>> +}
>> +
>> +/**
>> + * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
>> + * @pruss: pruss instance
>> + * @pru_id: PRU identifier (0-1)
>> + * @mux: new mux value for PRU
>> + *
>> + * Return: 0 on success, or an error code otherwise
>> + */
>> +static inline int pruss_cfg_set_gpmux(struct pruss *pruss,
>> +				      enum pruss_pru_id pru_id, u8 mux)
>> +{
>> +	if (mux >= PRUSS_GP_MUX_SEL_MAX ||
>> +	    pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>> +		return -EINVAL;
>> +
>> +	return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>> +				PRUSS_GPCFG_PRU_MUX_SEL_MASK,
>> +				(u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
>> +}
>> +
>>  #endif	/* _PRUSS_DRIVER_H_ */
>
Romain Naour Sept. 15, 2022, 8:27 a.m. UTC | #3 
Hello Roger,

Le 14/09/2022 à 15:15, Roger Quadros a écrit :
> Hello Romain,
> 
> On 12/09/2022 17:20, Romain Naour wrote:
>> +Danish
>>
>> Hi Danish,
>>
>> (Removed Puranjay (as he is no longer with TI) and adding Danish.)
>>
>> Le 18/04/2022 à 14:30, Puranjay Mohan a écrit :
>>> From: Tero Kristo <t-kristo@ti.com>
>>>
>>> Add two new helper functions pruss_cfg_get_gpmux() & pruss_cfg_set_gpmux()
>>> to get and set the GP MUX mode for programming the PRUSS internal wrapper
>>> mux functionality as needed by usecases.
>>
>> Actually I'm curious about how the GP MUX mode are supposed to work in some
>> cases. The register mapping in the AM57xx TRM seems confusing.
>>
>> See the "PRU-ICSS I/O Interface" part about the "PRU-ICSS Internal Wrapper
>> Multiplexing" [1].
>>
>> The commit "ARM: dts: am57xx-idk: Add prueth on ICSS" [2] (only in the
>> TI kernel tree) adds pruss1 and pruss2 for the am571x-idk board.
>>
>> But this commit doesn't really explain the ti,pruss-gp-mux-sel setting
>> from pruss1_eth and pruss2_eth:
>>
>>     /* Dual mac ethernet application node on icss1 */
>>     pruss1_eth {
>>     	status = "okay";
>>     	compatible = "ti,am57-prueth";
>>
>>     	ti,pruss-gp-mux-sel = <0>,	/* GP, default */
>>     			      <4>;	/* MII2, needed for PRUSS1_MII1 */
>>     }
>>
>>     &pruss2_eth {
>>     	ti,pruss-gp-mux-sel = <4>,	/* MII2, needed for PRUSS1_MII0 */
>>     			      <4>;	/* MII2, needed for PRUSS1_MII1 */
>>     };
>>
>> At the first look, the two comments in pruss2_eth node about PRUSS1_MIIx seems
>> dubious. Indeed, it would means that the PRUSS2 setting (ti,pruss-gp-mux-sel) is
>> required to makes PRUSS1 work.
> 
> Yes, if I remember right this is only applicable to AM571 Soc which had
> an overloaded pinmuxing design and it resulted in this kind of weird constraint.
> i.e. gp-mux-sel of PRUSS2 needs to be set to get PRUSS1 MII to work.

Thanks for you feed back!

From my experience the "issue" is also on the AM5749 design.

I haven't find any comment or warning from TI in the datasheet, the devicetree
or the git log about this weird constraint.

I was wondering why the PRUSS2 was used on the IDK574x board out of the box [1].
I did some hardware changes to be abble to test the PRUSS1 with prueth driver. I
was able to test an confirm that PRUSS1 was working well but the PRUSS2 was
still working.

But on my custom board, only the PRUSS1 was used with the prueth driver... so it
was not working as expected.

At least the pruss driver should check for such non working setting :-/

[1] https://www.ti.com/tool/TMDSIDK574

Best regards,
Romain


> 
>>
>> In my use case, only the pruss1 is expected to be used with the prueth driver.
>>
>> Actually, the prueth on PRUSS1 partially works with only pruss1_eth's gp-mux
>> initialized:
>>
>>     pruss1_eth {
>>             status = "okay";
>>             compatible = "ti,am57-prueth";
>>
>>             ti,pruss-gp-mux-sel = <0>,      /* GP, default */
>>                                   <4>;      /* MII2, needed for PRUSS1_MII1 */
>>     }
>>
>>     pruss2_eth {
>>             status = "disabled";
>>     }
>>
>> (Tests done with the ti-linux-kernel 5.10.y)
>>
>> On wireshark I noticed ethernet frames (ping) sent from the board but the reply
>> from the remote PC is never received on the board.
>>
>> It really seems we need pruss2_eth's gp-mux initialized.
>> The problem here is that I don't want to enable PRUSS2 just to
>> configure pruss2_eth's gp-mux for the sake of pruss1.
>>
>> I had to write manually (using devmem2) the "good" value (0x10002003) in
>> PRUSS2_CFG0 and PRUSS2_CFG1 to configure entirely the PRUSS1_MII1.
>>
>> I'm not sure how the driver should handle this register mapping properly.
>>
>> [1] https://www.ti.com/lit/ds/symlink/am5749.pdf
>>
>> [2]
>> https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel/commit/?h=linux-5.10.y&id=2a3b089f5697fe2f9a9875b2fba1bef88d196a53
>>
>> Best regards,
>> Romain
> 
> cheers,
> -roger
> 
>>
>>>
>>> Co-developed-by: Suman Anna <s-anna@ti.com>
>>> Signed-off-by: Suman Anna <s-anna@ti.com>
>>> Signed-off-by: Tero Kristo <t-kristo@ti.com>
>>> Co-developed-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@linaro.org>
>>> Signed-off-by: Puranjay Mohan <p-mohan@ti.com>
>>> ---
>>>  include/linux/pruss_driver.h | 44 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 44 insertions(+)
>>>
>>> diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
>>> index e2d5477225c6..3312281ef4c1 100644
>>> --- a/include/linux/pruss_driver.h
>>> +++ b/include/linux/pruss_driver.h
>>> @@ -35,4 +35,48 @@ struct pruss {
>>>  	struct clk *iep_clk_mux;
>>>  };
>>>  
>>> +/**
>>> + * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
>>> + * @pruss: pruss instance
>>> + * @pru_id: PRU identifier (0-1)
>>> + * @mux: pointer to store the current mux value into
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +static inline int pruss_cfg_get_gpmux(struct pruss *pruss,
>>> +				      enum pruss_pru_id pru_id, u8 *mux)
>>> +{
>>> +	int ret = 0;
>>> +	u32 val;
>>> +
>>> +	if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>> +		return -EINVAL;
>>> +
>>> +	ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
>>> +	if (!ret)
>>> +		*mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
>>> +			    PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
>>> +	return ret;
>>> +}
>>> +
>>> +/**
>>> + * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
>>> + * @pruss: pruss instance
>>> + * @pru_id: PRU identifier (0-1)
>>> + * @mux: new mux value for PRU
>>> + *
>>> + * Return: 0 on success, or an error code otherwise
>>> + */
>>> +static inline int pruss_cfg_set_gpmux(struct pruss *pruss,
>>> +				      enum pruss_pru_id pru_id, u8 mux)
>>> +{
>>> +	if (mux >= PRUSS_GP_MUX_SEL_MAX ||
>>> +	    pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
>>> +		return -EINVAL;
>>> +
>>> +	return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
>>> +				PRUSS_GPCFG_PRU_MUX_SEL_MASK,
>>> +				(u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
>>> +}
>>> +
>>>  #endif	/* _PRUSS_DRIVER_H_ */
>>
diff mbox series

Patch

diff --git a/include/linux/pruss_driver.h b/include/linux/pruss_driver.h
index e2d5477225c6..3312281ef4c1 100644
--- a/include/linux/pruss_driver.h
+++ b/include/linux/pruss_driver.h
@@ -35,4 +35,48 @@  struct pruss {
 	struct clk *iep_clk_mux;
 };
 
+/**
+ * pruss_cfg_get_gpmux() - get the current GPMUX value for a PRU device
+ * @pruss: pruss instance
+ * @pru_id: PRU identifier (0-1)
+ * @mux: pointer to store the current mux value into
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+static inline int pruss_cfg_get_gpmux(struct pruss *pruss,
+				      enum pruss_pru_id pru_id, u8 *mux)
+{
+	int ret = 0;
+	u32 val;
+
+	if (pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
+		return -EINVAL;
+
+	ret = pruss_cfg_read(pruss, PRUSS_CFG_GPCFG(pru_id), &val);
+	if (!ret)
+		*mux = (u8)((val & PRUSS_GPCFG_PRU_MUX_SEL_MASK) >>
+			    PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
+	return ret;
+}
+
+/**
+ * pruss_cfg_set_gpmux() - set the GPMUX value for a PRU device
+ * @pruss: pruss instance
+ * @pru_id: PRU identifier (0-1)
+ * @mux: new mux value for PRU
+ *
+ * Return: 0 on success, or an error code otherwise
+ */
+static inline int pruss_cfg_set_gpmux(struct pruss *pruss,
+				      enum pruss_pru_id pru_id, u8 mux)
+{
+	if (mux >= PRUSS_GP_MUX_SEL_MAX ||
+	    pru_id < 0 || pru_id >= PRUSS_NUM_PRUS)
+		return -EINVAL;
+
+	return pruss_cfg_update(pruss, PRUSS_CFG_GPCFG(pru_id),
+				PRUSS_GPCFG_PRU_MUX_SEL_MASK,
+				(u32)mux << PRUSS_GPCFG_PRU_MUX_SEL_SHIFT);
+}
+
 #endif	/* _PRUSS_DRIVER_H_ */