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

[Roger Quadros <rogerq@kernel.org>]

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_ */
>