Received-SPF: pass (google.com: domain of linux-kernel+bounces-44438-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Sai Krishna Gajula <saikrishnag@marvell.com>
To: Simon Horman <horms@kernel.org>
CC: "richardcochran@gmail.com" <richardcochran@gmail.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "kuba@kernel.org"
	<kuba@kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sunil Kovvuri
 Goutham <sgoutham@marvell.com>,
        Geethasowjanya Akula <gakula@marvell.com>,
        Linu Cherian <lcherian@marvell.com>,
        Hariprasad Kelam <hkelam@marvell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@marvell.com>,
        Naveen Mamindlapalli
	<naveenm@marvell.com>
Subject: Re: [net-next PATCH] octeontx2: Add PTP clock driver for Octeon PTM
 clock.
Thread-Topic: [net-next PATCH] octeontx2: Add PTP clock driver for Octeon PTM
 clock.
Thread-Index: AQHaU2r/3q7bgzzZEEmTAPAsMLYhuQ==
Date: Tue, 30 Jan 2024 10:56:37 +0000
Message-ID: 
 <BY3PR18MB47075899686A57C3FBA0A7CFA07D2@BY3PR18MB4707.namprd18.prod.outlook.com>
References: <20240124064156.2577119-1-saikrishnag@marvell.com>
 <20240129105921.GJ401354@kernel.org>
In-Reply-To: <20240129105921.GJ401354@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?ZCkM1cMLEa5o6U5P2F1eR3JwOf3XHYtpp6OAOCgFQZO+2E908WD/j19yc4mq?=
 =?us-ascii?Q?zEML3AsmNukxzg+0qtq2BIhl9DhH2+F7aUUk63DSDoduFVgpJFy8NJ+KZzDs?=
 =?us-ascii?Q?Ua/MdcWR6ZFeie4SMUONJgPvMKxKKcN7vhd8HfL2FroVO2mKaudq+kUdU5uS?=
 =?us-ascii?Q?9xmi03wZGbVFxgkKwecBRk4nIR8s1e6ezTExX/y1m6LeeKZ7GFPq5Zps5qgK?=
 =?us-ascii?Q?BLEnGRELssJc7wYcP8syf8ExOnMf+rbpM0f4a9r2XMDwprLe5Y0/xQQfHzHW?=
 =?us-ascii?Q?LUNEQUlxv6uZACawJfrvyvEfIW7ood0z5vYaMK/FOpl6X3hacOtjDcgnEHv9?=
 =?us-ascii?Q?FXy1Ip9Y+7w0S4L6HYYnbKx3Qnyvg93y3/e27qr7x4aI9q+cssSrTacDsMlx?=
 =?us-ascii?Q?+lsai/VxlOf1ZqcH6K5SA+2DLoPvxEgTN24+4EAgqEMjIXYd7hZVxd2ZJUTL?=
 =?us-ascii?Q?PQjNpK+2groOhEBo9z7cGvEEVU8gzM69Bf/ABcKxFZPUZO/sEl6+hyNVoQhV?=
 =?us-ascii?Q?2OW6vZ1eANTJEH6ZHYkMgAFq8NeM0JpANaaq1v48tyQfQjZ+xvnWpp1Vli1Q?=
 =?us-ascii?Q?FnjF+lx15pq+CUBBr0MNZcfcPMwzcTJzi5OlJrh9vsWzKFS9M7TR3MDDBFH8?=
 =?us-ascii?Q?kulMDc2Bt+aDvZV6z67gXFZyagSw0mp1Q+aRNMSfIGiPoyuaWSLylrnbiPEt?=
 =?us-ascii?Q?LCWxWwhm7HJl9WU2UkjAAGj9eGRiVmAoNg5OFYVfgsAq4m8RGAhHfMNbkH5C?=
 =?us-ascii?Q?W2tYuycKg1Cv/HAEoM4Tvq4FBHbUNHrdO6Tg1+TtjK6fojF2Uj14eCFlNuaJ?=
 =?us-ascii?Q?Ax8ZYqZK3RCf47QlR3ry0e5QrxXICw3/2kU0k7Dmi2WxdpYKuqXau68O5uDL?=
 =?us-ascii?Q?R1JrmOLd0PA/YQy0Maf1betSIXIhsG9NV9YKvMc2+XaqOxVCzB9qF8v5vz6e?=
 =?us-ascii?Q?aQFgHvK4gLpIYCFHdb31IEtK+aRSef2zPZE4GIgFq3J1eQ5/6NCGN0yaBUVF?=
 =?us-ascii?Q?MIFbz1vZVgfX86WOL3qrL5tl3RtVwoDvB7JrWgFnHZ46viEBpHAxJcRxnh7S?=
 =?us-ascii?Q?ZhUzlD3c1wt7s1+T+YaQo1h842vq1mti4k/mcYW9WmlQmVcWHpEy8CqgXWtL?=
 =?us-ascii?Q?zpwt8R4KdGs/I4NeqifLU9An/FSwev5ZX+nFn9l1FTe0syJ/6VmiaiLYI9ow?=
 =?us-ascii?Q?goYusTQne8fGqxp8M3ZhJpy9n7SmdJZOUG+Wyec0QYOY631JDwlwRtaWrbzu?=
 =?us-ascii?Q?p1gptltgEMgHlimGozTC3FU10wpA8KCsuER/M2JcEO6aiU4YuZLdGoF//Wss?=
 =?us-ascii?Q?ySCnZGceuIP/Y42tWS9eDpdJGfSR6499Ld6ZCoOWMrusoNL/YcxmuBQ2HfF9?=
 =?us-ascii?Q?x2MavtW5UG6nBHkRlb9oAABpJWaNnzQqPFDo8c2jGRCwaFnA3qlgMsBtL0fR?=
 =?us-ascii?Q?/SUJ8otB3QqPPAjf/HIhNqY88EWiZIf32UpM+qavMMJzszzB4cGsb+7ynltA?=
 =?us-ascii?Q?MW9wBPhXcG5JwH7vgiigd+/YFOvq2iMFz2QuEXX5dj6ylF6oCWUZ7+FlvdQt?=
 =?us-ascii?Q?Ab312rx20UupALLMA4ol/TRVMvO7jlirPuMujjFC?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BY3PR18MB4707.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 79477646-3341-47f3-7e44-08dc218221dc
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Jan 2024 10:56:37.7264
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: JUVLcn4Ku+rm4Kdscjbhglusxwn0CM3+TLSvsSQnKnbUZsHCklDP8ciqEiEYpSXolxS1ZMAf5cE/fgOQZwb/iA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM8PR18MB4438
X-Proofpoint-ORIG-GUID: j5JCBsKKgBooRL1yOhwJIa2AtyxAifvp
X-Proofpoint-GUID: j5JCBsKKgBooRL1yOhwJIa2AtyxAifvp
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-30_05,2024-01-30_01,2023-05-22_02


> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Monday, January 29, 2024 4:29 PM
> To: Sai Krishna Gajula <saikrishnag@marvell.com>
> Cc: richardcochran@gmail.com; davem@davemloft.net; kuba@kernel.org;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Geethasowjanya Akula
> <gakula@marvell.com>; Linu Cherian <lcherian@marvell.com>; Hariprasad
> Kelam <hkelam@marvell.com>; Subbaraya Sundeep Bhatta
> <sbhatta@marvell.com>; Naveen Mamindlapalli <naveenm@marvell.com>
> Subject: Re: [net-next PATCH] octeontx2: Add PTP clock driver for
> Octeon PTM clock.
>=20
> On Wed, Jan 24, 2024 at 12:11:56PM +0530, Sai Krishna wrote:
> > The PCIe PTM(Precision time measurement) protocol provides precise
> > coordination of events across multiple components like PCIe host
> > clock, PCIe EP PHC local clocks of PCIe devices. This patch adds
> > support for ptp clock based PTM clock. We can use this PTP device to
> > sync the PTM time with CLOCK_REALTIME or other PTP PHC devices using
> > phc2sys.
> >
> > Signed-off-by: Sai Krishna <saikrishnag@marvell.com>
> > Signed-off-by: Naveen Mamindlapalli <naveenm@marvell.com>
> > Signed-off-by: Sunil Kovvuri Goutham <sgoutham@marvell.com>
>=20
> Hi Sai,
>=20
> some minor review items from my side.
>=20
> > diff --git a/drivers/ptp/ptp_octeon_ptm.c
> > b/drivers/ptp/ptp_octeon_ptm.c
>=20
> ...
>=20
> > +static u32 read_pcie_config32(int ep_pem, int cfg_addr) {
> > +	void __iomem *addr;
> > +	u64 val;
> > +
> > +	if (oct_ptp_clock.cn10k_variant) {
> > +		addr  =3D ioremap(PEMX_PFX_CSX_PFCFGX(ep_pem, 0,
> cfg_addr), 8);
> > +		if (!addr) {
> > +			pr_err("PTM_EP: Failed to ioremap Octeon CSR
> space\n");
> > +			return -1U;
> > +		}
> > +		val =3D readl(addr);
> > +		iounmap(addr);
> > +	} else {
> > +		addr  =3D ioremap(PEMX_CFG_RD(ep_pem), 8);
> > +		if (!addr) {
> > +			pr_err("PTM_EP: Failed to ioremap Octeon CSR
> space\n");
> > +			return -1U;
> > +		}
> > +		val =3D ((1 << 15) | (cfg_addr & 0xfff));
> > +		writeq(val, addr);
>=20
> This causes a build failure on x86_32 because writeq() is undefined.

Ack, Will add ARM64 dependency in Kconfig  (depends on (64BIT && COMPILE_TE=
ST) || ARM64).  Hope this works!!

>=20
> > +		val =3D readq(addr) >> 32;
> > +		iounmap(addr);
> > +	}
> > +	return (val & 0xffffffff);
> > +}
> > +
> > +static uint64_t octeon_csr_read(u64 csr_addr) {
> > +	u64 val;
> > +	void __iomem *addr;
>=20
> nit: In Networking code, please consider arranging local variables
>      in reverse xmas tree order - longest line to shortest.

Ack,  yeah missed this.. will change in patch V2.

>=20
> > +
> > +	addr =3D ioremap(csr_addr, 8);
> > +	if (!addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return -1UL;
> > +	}
> > +	val =3D (u64)READ_ONCE(*(u64 __iomem *)addr);
>=20
> Sparse seems unhappy about this cast.
> So if this is really what you want to do then probably a __force is neede=
d in
> the cast.
>=20
> But I do wonder if there is an endian consideration that needs to be take=
n
> care of here. And, moreover, if a standard routine, such as ioread64(), c=
ould
> be used instead of this function.
>=20
> N.B. as per the note on writeq, possibly this only works on 64bit systems=
.
>=20
> Likewise elsewhere in this patch.
>=20

Ack,  will submit the changes in patch V2.

> > +	iounmap(addr);
> > +	return val;
> > +}
>=20
> ...
>=20
> > +static int __init ptp_oct_ptm_init(void) {
> > +	struct pci_dev *pdev =3D NULL;
> > +
> > +	pdev =3D pci_get_device(PCI_VENDOR_ID_CAVIUM,
> > +			      PCI_DEVID_OCTEONTX2_PTP, pdev);
> > +	if (!pdev)
> > +		return 0;
> > +
> > +	if (octeon_csr_read(PEMX_CFG) & 0x1ULL) {
> > +		pr_err("PEM0 is configured as RC\n");
> > +		return 0;
> > +	}
> > +
> > +	if (is_otx2_support_ptm(pdev)) {
> > +		oct_ptp_clock.cn10k_variant =3D 0;
> > +	} else if (is_cn10k_support_ptm(pdev)) {
> > +		oct_ptp_clock.cn10k_variant =3D 1;
> > +	} else {
> > +		/* PTM_EP: unsupported processor */
> > +		return 0;
> > +	}
> > +
> > +	ptm_ctl_addr =3D ioremap(PEMX_PTM_CTL, 8);
> > +	if (!ptm_ctl_addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return 0;
> > +	}
> > +
> > +	ptm_lcl_addr =3D ioremap(PEMX_PTM_LCL_TIME, 8);
> > +	if (!ptm_lcl_addr) {
> > +		pr_err("PTM_EP: Failed to ioremap CSR space\n");
> > +		return 0;
> > +	}
> > +
> > +	oct_ptp_clock.caps =3D ptp_oct_caps;
> > +
> > +	oct_ptp_clock.ptp_clock =3D ptp_clock_register(&oct_ptp_clock.caps,
> > +NULL);
> > +
> > +	pr_info("PTP device index for PTM clock:%d\n",
> > +oct_ptp_clock.ptp_clock->index);
>=20
> It seems that the pr_info() call above assumes that oct_ptp_clock.ptp_clo=
ck
> is not an error, but it may be.
>=20
> Perhaps something like this is more appropriate:
>=20
> 	oct_ptp_clock.ptp_clock =3D ...
> 	if (IS_ERR(oct_ptp_clock.ptp_clock))
> 		ERR_PTR(oct_ptp_clock.ptp_clock);
>=20
> 	pr_info(...)
> 	...
>=20
> 	return 0;
>=20
> Flagged by Smatch.

Ack,  will submit the changes in patch V2.

>=20
> > +	pr_info("cn10k_variant %d\n", oct_ptp_clock.cn10k_variant);
> > +
> > +	return PTR_ERR_OR_ZERO(oct_ptp_clock.ptp_clock);
> > +}
> > +
> > +module_init(ptp_oct_ptm_init);
> > +module_exit(ptp_oct_ptm_exit);
> > +
> > +MODULE_AUTHOR("Marvell Inc.");
> > +MODULE_DESCRIPTION("PTP PHC clock using PTM");
> MODULE_LICENSE("GPL");
> > --
> > 2.25.1


Thanks,
Sai