Received-SPF: pass (google.com: domain of linux-kernel+bounces-8627-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 21 Dec 2023 10:14:31 -0500
From: Frank Li <Frank.li@nxp.com>
To: Joy Zou <joy.zou@nxp.com>
Cc: "vkoul@kernel.org" <vkoul@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"imx@lists.linux.dev" <imx@lists.linux.dev>,
	"krzysztof.kozlowski+dt@linaro.org" <krzysztof.kozlowski+dt@linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Peng Fan <peng.fan@nxp.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	Shenwei Wang <shenwei.wang@nxp.com>
Subject: Re: [PATCH v3 6/6] dmaengine: fsl-edma: integrate TCD64 support for
 i.MX95
Message-ID: <ZYRWV+tRXCrP7t0p@lizhi-Precision-Tower-5810>
References: <20231127225542.2744711-1-Frank.Li@nxp.com>
 <20231127225542.2744711-7-Frank.Li@nxp.com>
 <AM6PR04MB5925D78A39F3CF7F9273ECEBE195A@AM6PR04MB5925.eurprd04.prod.outlook.com>
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <AM6PR04MB5925D78A39F3CF7F9273ECEBE195A@AM6PR04MB5925.eurprd04.prod.outlook.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?RzU3SnBQWWcxeUkxTDhOZ0pRWjRhbStLaDZra3dldXBXZGQxU3pPYjdkY1JM?=
 =?utf-8?B?QVo1MjVCaVBBVkp5OU5NTGRHZ281SnZRblZZUi94UEI1akpGSHpQL29NN1Fs?=
 =?utf-8?B?VW5jcm83QWg3RHhXMFdsNm5mR25FRjEzc0hwZ3F1UW9KZERVYjhkb1o1MGlX?=
 =?utf-8?B?UjIwNkNlb296aGpIV21xeEJEUTgySlF1WUFicy8wcmU3T04za2J3SW1CdCs0?=
 =?utf-8?B?Q2FOQVo0dURIYTk4MkVyU1h1THVNeGNOcUt5KzRMM2tVZEJEekxVRFczQ0Mw?=
 =?utf-8?B?TFhrVG9BWEx0OU5QZnBPM3JUOXlLWllrOEtTK1pIekJTSzd2OUdaMlo4WFEw?=
 =?utf-8?B?eFcxY1pIZUswdWxOT21zN2F5RnVrM2JleitRWjJMZE1mSHhnYXd4S3Zzczd6?=
 =?utf-8?B?QzhYaE55Q3ZYVjZUeU8wVkczNk9lMHhoS1V6cHJBdTBLbW1wZTIzUVVyOG5h?=
 =?utf-8?B?ZXNPU2hWVGlxanF5TkJJSmJ5amYydk1pRy96eHkwK1cxODZGb3p0YlhGQzQv?=
 =?utf-8?B?L3BvVFkyY2hmWDZvZTk2SEdCbjhPT0ErSzlVY3AxVjdFYjBTWTZJSVFOSCtx?=
 =?utf-8?B?eEhNVGNic3k0YmdYS05jVTVlSnFBdHlQNktPM3J1dTloeFJDTVFLbXBHcVJS?=
 =?utf-8?B?cStNVzNBLzg0eGxTL3U3Y3VQNUhjd2kxbVN5YjVwSkZPUlBhRGl0T1BySnFY?=
 =?utf-8?B?NXErTm9NRjRqbWsrL1VkV2dlWGFwd3pnVnN5WjRaMU9Pb1F1M3JvdnVzcUcw?=
 =?utf-8?B?Y1piOFpjVWNNN25tM0NsRjdIcjBIdTU2dXNvNEM2ZEpkSTQ1NUNobHcrRjFk?=
 =?utf-8?B?dWJWa0dzZS9hbTJRZXhwdUIvbnU2N1I4d0RQV1d3YUVkb2FNM2NjOXZ5cDIz?=
 =?utf-8?B?TmR6RENtNlFmdGw2NDBUZDZlSS83SDFtVS9ISnlSdHkrQTB5SWdrMm8yaTFX?=
 =?utf-8?B?MTJrY0hjeWRuSW4vZFFNVXZqcUoyYnMyK2xpOTErY1R0RE0vMTkremZRQzFX?=
 =?utf-8?B?UjFBVS9oV1FvN0pXRE52c2EyYWNCaUVRbm9SdXBrWnJ1elNLczErUkp3ek9Q?=
 =?utf-8?B?SXlFZldNVG1SYmk5d1VyYmFpbkpGWWhzSzhkREpKQk4vS1JzYkFsUTJsY0Rs?=
 =?utf-8?B?N1hkWi9EaC9BaDliV21WWDlHZnMrTThwNkh1SVI3d2x1QnA1NHN6QllHRUhp?=
 =?utf-8?B?b29CQW5UdE5TQzNwRHg2elpMOTMvTURLN2hSSDNXeFBhVUJPVHM1aUxaaHRQ?=
 =?utf-8?B?c1RnbU0xUWswVFNYUVNvTDhnR3V4TnozakVVT0YzVHdySW11dVZqR3ZsYXhy?=
 =?utf-8?B?ejZaamlTRkloNGFDdnpqaVpkTThMdy95SmtlSlV4U3NScWlUL0l2M25tVzdD?=
 =?utf-8?B?NlU4U29DZ0FhK3FqQ1k5aHlMZTNlSWJ1SHlxdjhTZHg0c1Z2a2dSZ3Zjb2sy?=
 =?utf-8?B?TWtwbWU5WUpTU3Avd3RESWlsS2xXVHhGYmVwb2dqRThqWHlBWk5XTWt5alNQ?=
 =?utf-8?B?M29ZZlVuL0pDSEs4S2J3N3FjMjM5Z0hlckgwYW1wZGUzVGF6eGV2ZlZibGdt?=
 =?utf-8?B?UGRISmg5VW5zQmw2N2tjanBkQnEvTEFHclVEVERhWEM3ZFNrUTRlMUdGUUFJ?=
 =?utf-8?B?YWRJK1pJTWl2STVTVDMzT3hpL1B6OFhML2x0U2JzVVk3eXFBK00vZEJGTE1v?=
 =?utf-8?B?Vy9jN1NLSVZOOU5VVE5mTUF0bFlscjNuenh2dDI2MWdvanh5KzhUS2Q2TVZM?=
 =?utf-8?B?YWg4SFpJb2dDSzZ5Q25lMitzbHlrQlp3aTB4Mk1EZC9xcDB2SVlHdWgxV3JQ?=
 =?utf-8?B?QWJBcXRDT1ZhQkpDWmEzWFJOOVlYQXAxdStqRVJTTENBNG5DMGlyZGlXbzlr?=
 =?utf-8?B?cDRrWHF0eFcvMDhQRklTMS9zaWQ0VXJFSTRMU2oxd3k4OUZiUHFHcURucXho?=
 =?utf-8?B?eUxVZ3p1Tkhya2p4eVVGblBLVmVQZEpoclcxaDV5TGw3QThzLzJJb2w0RGd1?=
 =?utf-8?B?UWpYckxDZTdiUnlvMFdCcCszRHB0WVVJTlp2dzlZT0ZkemRUS3oyUTZ5NWRN?=
 =?utf-8?B?RVJuR1Y2ZU1jd3hWV0FaRHQwL0JPczlEUEtLc29Nb2hNUFIzMFM5UjloOW56?=
 =?utf-8?Q?7gYM=3D?=
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0e514ea0-d460-4192-affe-08dc02378ca1
X-MS-Exchange-CrossTenant-AuthSource: AM6PR04MB4838.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Dec 2023 15:14:38.7020
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: IFPFQ/UIl3glt98zXNK+8Ydnvo3RAAMm4R3zylcJQrJQsvh8/KoKTffEhf9nMm9aqUoX5SGYcAdgXHa1ZfZOSw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DBBPR04MB7529

On Thu, Dec 21, 2023 at 07:47:19AM +0000, Joy Zou wrote:
> Hi frank,
> > -----Original Message-----
> > From: Frank Li <frank.li@nxp.com>
> > Sent: 2023年11月28日 6:56
> > To: Frank Li <frank.li@nxp.com>; vkoul@kernel.org
> > Cc: devicetree@vger.kernel.org; dmaengine@vger.kernel.org;
> > imx@lists.linux.dev; Joy Zou <joy.zou@nxp.com>;
> > krzysztof.kozlowski+dt@linaro.org; linux-kernel@vger.kernel.org; Peng Fan
> > <peng.fan@nxp.com>; robh+dt@kernel.org; Shenwei Wang
> > <shenwei.wang@nxp.com>
> > Subject: [PATCH v3 6/6] dmaengine: fsl-edma: integrate TCD64 support for
> > i.MX95
> > 
> > In i.MX95's edma version 5, the TCD structure is extended to support 64-bit
> > addresses for fields like saddr and daddr. To prevent code duplication, employ
> > help macros to handle the fields, as the field names remain the same between
> > TCD and TCD64.
> > 
> > Change local variables related to TCD addresses from 'u32' to 'dma_addr_t'
> > to accept 64-bit DMA addresses.
> > 
> > Change 'vtcd' type to 'void *' to avoid direct use. Use helper macros to access
> > the TCD fields correctly.
> > 
> > Call 'dma_set_mask_and_coherent(64)' when TCD64 is supported.
> > 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/dma/fsl-edma-common.c |  34 ++++---
> > drivers/dma/fsl-edma-common.h | 165
> > +++++++++++++++++++++++++++-------
> >  drivers/dma/fsl-edma-main.c   |  14 +++
> >  3 files changed, 170 insertions(+), 43 deletions(-)
> > 
> > diff --git a/drivers/dma/fsl-edma-common.c
> > b/drivers/dma/fsl-edma-common.c index 65f466ab9d4da..c8acff09308fd
> > 100644
> > --- a/drivers/dma/fsl-edma-common.c
> > +++ b/drivers/dma/fsl-edma-common.c
> > @@ -351,7 +351,7 @@ static size_t fsl_edma_desc_residue(struct
> > fsl_edma_chan *fsl_chan,  {
> >  	struct fsl_edma_desc *edesc = fsl_chan->edesc;
> >  	enum dma_transfer_direction dir = edesc->dirn;
> > -	dma_addr_t cur_addr, dma_addr;
> > +	dma_addr_t cur_addr, dma_addr, old_addr;
> >  	size_t len, size;
> >  	u32 nbytes = 0;
> >  	int i;
> > @@ -367,10 +367,16 @@ static size_t fsl_edma_desc_residue(struct
> > fsl_edma_chan *fsl_chan,
> >  	if (!in_progress)
> >  		return len;
> > 
> > -	if (dir == DMA_MEM_TO_DEV)
> > -		cur_addr = edma_read_tcdreg(fsl_chan, saddr);
> > -	else
> > -		cur_addr = edma_read_tcdreg(fsl_chan, daddr);
> > +	/* 64bit read is not atomic, need read retry when high 32bit changed */
> > +	do {
> > +		if (dir == DMA_MEM_TO_DEV) {
> > +			old_addr = edma_read_tcdreg(fsl_chan, saddr);
> > +			cur_addr = edma_read_tcdreg(fsl_chan, saddr);
> > +		} else {
> > +			old_addr = edma_read_tcdreg(fsl_chan, daddr);
> > +			cur_addr = edma_read_tcdreg(fsl_chan, daddr);
> > +		}
> > +	} while (upper_32_bits(cur_addr) != upper_32_bits(old_addr));
> > 
> >  	/* figure out the finished and calculate the residue */
> >  	for (i = 0; i < fsl_chan->edesc->n_tcds; i++) { @@ -426,8 +432,7 @@
> > enum dma_status fsl_edma_tx_status(struct dma_chan *chan,
> >  	return fsl_chan->status;
> >  }
> > 
> > -static void fsl_edma_set_tcd_regs(struct fsl_edma_chan *fsl_chan,
> > -				  struct fsl_edma_hw_tcd *tcd)
> > +static void fsl_edma_set_tcd_regs(struct fsl_edma_chan *fsl_chan, void
> > +*tcd)
> >  {
> >  	u16 csr = 0;
> > 
> > @@ -478,9 +483,9 @@ static void fsl_edma_set_tcd_regs(struct
> > fsl_edma_chan *fsl_chan,
> > 
> >  static inline
> >  void fsl_edma_fill_tcd(struct fsl_edma_chan *fsl_chan,
> > -		       struct fsl_edma_hw_tcd *tcd, u32 src, u32 dst,
> > -		       u16 attr, u16 soff, u32 nbytes, u32 slast, u16 citer,
> > -		       u16 biter, u16 doff, u32 dlast_sga, bool major_int,
> > +		       struct fsl_edma_hw_tcd *tcd, dma_addr_t src, dma_addr_t
> > dst,
> > +		       u16 attr, u16 soff, u32 nbytes, dma_addr_t slast, u16 citer,
> > +		       u16 biter, u16 doff, dma_addr_t dlast_sga, bool major_int,
> >  		       bool disable_req, bool enable_sg)  {
> >  	struct dma_slave_config *cfg = &fsl_chan->cfg; @@ -581,8 +586,9 @@
> > struct dma_async_tx_descriptor *fsl_edma_prep_dma_cyclic(
> >  	dma_addr_t dma_buf_next;
> >  	bool major_int = true;
> >  	int sg_len, i;
> > -	u32 src_addr, dst_addr, last_sg, nbytes;
> > +	dma_addr_t src_addr, dst_addr, last_sg;
> >  	u16 soff, doff, iter;
> > +	u32 nbytes;
> > 
> >  	if (!is_slave_direction(direction))
> >  		return NULL;
> > @@ -654,8 +660,9 @@ struct dma_async_tx_descriptor
> > *fsl_edma_prep_slave_sg(
> >  	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> >  	struct fsl_edma_desc *fsl_desc;
> >  	struct scatterlist *sg;
> > -	u32 src_addr, dst_addr, last_sg, nbytes;
> > +	dma_addr_t src_addr, dst_addr, last_sg;
> >  	u16 soff, doff, iter;
> > +	u32 nbytes;
> >  	int i;
> > 
> >  	if (!is_slave_direction(direction))
> > @@ -804,7 +811,8 @@ int fsl_edma_alloc_chan_resources(struct dma_chan
> > *chan)
> >  	struct fsl_edma_chan *fsl_chan = to_fsl_edma_chan(chan);
> > 
> >  	fsl_chan->tcd_pool = dma_pool_create("tcd_pool", chan->device->dev,
> > -				sizeof(struct fsl_edma_hw_tcd),
> > +				fsl_edma_drvflags(fsl_chan) & FSL_EDMA_DRV_TCD64 ?
> > +				sizeof(struct fsl_edma_hw_tcd64) : sizeof(struct
> > fsl_edma_hw_tcd),
> >  				32, 0);
> >  	return 0;
> >  }
> > diff --git a/drivers/dma/fsl-edma-common.h
> > b/drivers/dma/fsl-edma-common.h index 4f39a548547a6..6afceb9fded1b
> > 100644
> > --- a/drivers/dma/fsl-edma-common.h
> > +++ b/drivers/dma/fsl-edma-common.h
> > @@ -87,6 +87,20 @@ struct fsl_edma_hw_tcd {
> >  	__le16	biter;
> >  };
> > 
> > +struct fsl_edma_hw_tcd64 {
> > +	__le64  saddr;
> > +	__le16  soff;
> > +	__le16  attr;
> > +	__le32  nbytes;
> > +	__le64  slast;
> > +	__le64  daddr;
> > +	__le64  dlast_sga;
> > +	__le16  doff;
> > +	__le16  citer;
> > +	__le16  csr;
> > +	__le16  biter;
> > +} __packed;
> > +
> >  struct fsl_edma3_ch_reg {
> >  	__le32	ch_csr;
> >  	__le32	ch_es;
> > @@ -96,7 +110,10 @@ struct fsl_edma3_ch_reg {
> >  	__le32	ch_mux;
> >  	__le32  ch_mattr; /* edma4, reserved for edma3 */
> >  	__le32  ch_reserved;
> > -	struct fsl_edma_hw_tcd tcd;
> > +	union {
> > +		struct fsl_edma_hw_tcd tcd;
> > +		struct fsl_edma_hw_tcd tcd64;
> > +	};
> >  } __packed; 
> The tcd64 should be fsl_edma_hw_tcd64?
> BR

With help macro, it is not imporant, only used to caculate ch_reg address
from tcd address.

Anyway, it should change to fsl_edma_hw_tcd64 for better maintaince. 

Frank

> Joy Zou 
> > 
> >  /*
> > @@ -125,7 +142,7 @@ struct edma_regs {
> > 
> >  struct fsl_edma_sw_tcd {
> >  	dma_addr_t			ptcd;
> > -	struct fsl_edma_hw_tcd		*vtcd;
> > +	void				*vtcd;
>