Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Piyush Malgujar <pmalgujar@marvell.com>
To:     Andrew Lunn <andrew@lunn.ch>
CC:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Chandrakala Chavva <cchavva@marvell.com>,
        Damian Eppel <deppel@marvell.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH] Marvell MDIO clock related changes.
Thread-Topic: [PATCH] Marvell MDIO clock related changes.
Thread-Index: Adhbzq0IscPzO/F8TYquG4fv15RHuQ==
Date:   Fri, 29 Apr 2022 14:14:00 +0000
Message-ID: <CH0PR18MB4193CF9786F80101D08A2431A3FC9@CH0PR18MB4193.namprd18.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?XPgqctmQcuBYh1MWbuK1Pm3YQi5qde1XF+WX9WTuZZb+0Mn+ICm6TDNIfmFp?=
 =?us-ascii?Q?bTwUvbZv09ASzH24EKq2ADsaIog/lgEau+4xRkPn6WR4wyrCw4uO0DkHLxeb?=
 =?us-ascii?Q?yIvnJr7aEUTq2yk6yfMxwIMuEn1sHJ2hJ+E6jFx3gn2K/tq0PnELe4qOTt4F?=
 =?us-ascii?Q?O/2DcfQNulPm373/zEmOQiCw5xgxZgag0mhfr90qJU6Y/kS7AzmnIXhM2WGQ?=
 =?us-ascii?Q?dhjuso2J9NsUZ3ZSZWk5ek8yZfTT7ObMggL4uFMMHPfB+zpAiSmG1kun9Os9?=
 =?us-ascii?Q?M1ZuCxQu+fYse1382HxfK/Sag5oQ4Gf+dXQpCYT9WWxzwGFCZk1uCasQot43?=
 =?us-ascii?Q?UU5mEFiWIjLGkZ88TTLOFNq5u1cek4q6sHgNCvfpwMpQLBjV/tto/yVXVEIu?=
 =?us-ascii?Q?S8Pcvip8dUbNbm2Nk18dm4zJBlSx3Q1wai+dNnm9ibFxbI+lXoM/p/6zqF3A?=
 =?us-ascii?Q?ZG3C3L8fwmEGPUK/RolfMcJr3x4/dQnFQ4B8JIGpkIFDdmnTb5wBDh9eFQFX?=
 =?us-ascii?Q?A3P/r09J08lcm9swm9xHF46cvAqn5VEYTdhBcQri45f2FMXAxVCQrH9T2pue?=
 =?us-ascii?Q?NJ3nwAuGUN3Qc4cN8YduOeoewx5ln8LVgoCESwqXoQPFFpBUbBuFcqBCRd8p?=
 =?us-ascii?Q?FY9VPBdYygTDDiqteVR0ym4jAxA2b7fcm5AQnVdmDeC/h7MoaSAE8OV9oQct?=
 =?us-ascii?Q?splCiujK2TTkOqMf5GQme4W6djZZwSyraMtQLlYdwFcKAU+YWuAStqpk2QFY?=
 =?us-ascii?Q?xV8WZGcr12TiPnD/PLwTCYRfW+gW0Ar3GHiWf089jSC4mphXVOQb6+lV81Re?=
 =?us-ascii?Q?zF8JhJT8CNDeCMjiL6ZytkwgPrqxNBGHmifITOW9hJY6TIumtF0sZI7JUHzO?=
 =?us-ascii?Q?o/v+gTGB4UD3sAsTSOBoakIgrSWPyePPI2w9QDfa00i/S2ho2N07ISzsyY6l?=
 =?us-ascii?Q?gIAdmrxcHFuuqN8A/5rsXxlS4qZJ+T1V3higJ94Np1R2zr0q9yOVD7J1w90e?=
 =?us-ascii?Q?HU8iOTRbdShSxEma8H3RG32KLGitWQhPRMRzClWdOwPhtvYheMSvqQXTVekl?=
 =?us-ascii?Q?LX2xPWrS/BMRwTeWCyevsJ6a4JUGXMkj+05Xlz3ub7NGEGph1T99FkMfYOIx?=
 =?us-ascii?Q?rQEsimlJiHmMubbSmva92KJ+PWjI+bLGLYZbf6C8NL5cjKHpOIP4grECxsDj?=
 =?us-ascii?Q?cg8p1vcG1n7mV9BPOOIbfMsxBTgVS+aKJxv4CJ5+xqFpFIk/Gv7fmhgRdk1O?=
 =?us-ascii?Q?NPsE4z9ujoUH1muhjEt72tHsEyQXTYOA09hiDMqywaSSAmXKcC14/9pjHuiI?=
 =?us-ascii?Q?hhBO2YfyOUmfG2oBpadIMPw7gtCDtD0R/yWuGNIGujVI8UZ031Ic4fLqm4tZ?=
 =?us-ascii?Q?8cFpBO3E+Rj2kcv0b09nIjiRGWllDMuudCXC95UQTSdpHH1zoAcJS6jAS6lL?=
 =?us-ascii?Q?PbmIxBqqe9xfkGszS90xN7sOqpyK7EZeevhVkygQ3EOLjz0jAp3GzUoOJDEh?=
 =?us-ascii?Q?Frzh5KqJE26vajOBiyUQrEifL4TZWMWsY8eAtOP4faD9ekBqVMqxDh3PQB2E?=
 =?us-ascii?Q?1jQWC0SCwH7mr7gDcexCG1wOCEYk5Wwm6giy/NVf26UQ4pSyQ10mbdiK7iqR?=
 =?us-ascii?Q?mscPowMDQZapL5Gddu2LCqKulPDbwbwbR5NM1N6ZBYclb/XmMpXggnGKUBcQ?=
 =?us-ascii?Q?DMHU9MI+iAVUdezd4VlzTMMF4qIPCqrDm+VvE/JIrm+Awz1TOI5wtLwvJg53?=
 =?us-ascii?Q?hkV8mFvPWQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH0PR18MB4193.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 77153768-554f-44cd-6c85-08da29ea821c
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Apr 2022 14:14:00.8177
 (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: biWlR3OtSTqWcr2YcJDy1BvIlj3MpukzVRArEqsuHkFBO9Fd7RRV0lbjZaG3uiOuy4VjbggDQQ/BZ9A+zVkauQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR18MB3320
X-Proofpoint-ORIG-GUID: FA2a3FIxTnA-ouAsX5KcFT7g640jlaYR
X-Proofpoint-GUID: FA2a3FIxTnA-ouAsX5KcFT7g640jlaYR
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.858,Hydra:6.0.486,FMLib:17.11.64.514
 definitions=2022-04-29_07,2022-04-28_01,2022-02-23_01
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andrew,

Firstly, thanks for your time to review this patch.

> > This patch includes following support related to MDIO Clock:
>=20
> Please make you patch subject more specific. Marvell has lots of MDIO bus
> masters, those in the mvebu SoCs, there is at least one USB MDIO bus
> master, a couple in various SoHo switches, etc.
>=20
> git log --oneline mdio-thunder.c will give you an idea what to use.
>=20

This patch is for Marvell OcteonTX and CN10k hardware, this is
different from mvebu SoCs. I will add it in subject line for V2 version.

> > 1) clock gating:
> > The purpose of this change is to apply clock gating for MDIO clock when
> there is no transaction happening.
> > This will stop the MDC clock toggling in idle scenario.
> >
> > 2) Marvell MDIO clock frequency attribute change:
> > This MDIO change provides an option for user to have the bus speed set
> > to their needs which is otherwise set to default(3.125 MHz).
>=20
> Please read 802.3 Clause 22. The default should be 2.5MHz.
>=20

These changes are only specific to Marvell Octeon family.

> Also, you are clearly doing two different things here, so there should be=
 two
> patches.
>=20

Sure, I will create separate patches in V2 version.

> > In case someone needs to use this attribute, they have to add an extra
> > attribute clock-freq in the mdio entry in their DTS and this driver wil=
l
> support the rest.
> >
> > The changes are made in a way that the clock will set to the nearest
> > possible value based on the clock calculation
>=20
> Please keep line lengths to 80. I'm surprised checkpatch did not warn abo=
ut
> this.
>=20
>=20
> > and required frequency from DTS. Below are some possible values:
> > default:3.125 MHz
> > Max:16.67 MHz
> >
> > These changes has been verified internally with Marvell SoCs 9x and 10x
> series.
> >
> > Signed-off-by: Piyush Malgujar <pmalgujar@marvell.com>
> > Signed-off-by: Damian Eppel <deppel@marvell.com>
>=20
> These are in the wrong order. Since you are submitting it, your
> Signed-off-by: comes last.
>=20
> > ---
> >  drivers/net/mdio/mdio-cavium.h  |  1 +
> > drivers/net/mdio/mdio-thunder.c | 65
> +++++++++++++++++++++++++++++++++
> >  2 files changed, 66 insertions(+)
> >
> > diff --git a/drivers/net/mdio/mdio-cavium.h
> > b/drivers/net/mdio/mdio-cavium.h index
> >
> a2245d436f5dae4d6424b7c7bfca0aa969a3b3ad..ed4c48d8a38bd80e6a169f7
> a6d90
> > c1f2a0daccfc 100644
> > --- a/drivers/net/mdio/mdio-cavium.h
> > +++ b/drivers/net/mdio/mdio-cavium.h
> > @@ -92,6 +92,7 @@ struct cavium_mdiobus {
> >  	struct mii_bus *mii_bus;
> >  	void __iomem *register_base;
> >  	enum cavium_mdiobus_mode mode;
> > +	u32 clk_freq;
> >  };
> >
> >  #ifdef CONFIG_CAVIUM_OCTEON_SOC
> > diff --git a/drivers/net/mdio/mdio-thunder.c
> > b/drivers/net/mdio/mdio-thunder.c index
> >
> 822d2cdd2f3599025f3e79d4243337c18114c951..642d08aff3f7f849102992a89
> 179
> > 0e900b111d5c 100644
> > --- a/drivers/net/mdio/mdio-thunder.c
> > +++ b/drivers/net/mdio/mdio-thunder.c
> > @@ -19,6 +19,46 @@ struct thunder_mdiobus_nexus {
> >  	struct cavium_mdiobus *buses[4];
> >  };
> >
> > +#define _calc_clk_freq(_phase) (100000000U / (2 * (_phase))) #define
> > +_calc_sample(_phase) (2 * (_phase) - 3)
>=20
> Please avoid macros like this. Use a function.
>=20
> > +
> > +#define PHASE_MIN 3
> > +#define PHASE_DFLT 16
> > +#define DFLT_CLK_FREQ _calc_clk_freq(PHASE_DFLT) #define
> MAX_CLK_FREQ
> > +_calc_clk_freq(PHASE_MIN)
> > +
> > +static inline u32 _config_clk(u32 req_freq, u32 *phase, u32 *sample)
> > +{
> > +	unsigned int p;
> > +	u32 freq =3D 0, freq_prev;
> > +
> > +	for (p =3D PHASE_MIN; p < PHASE_DFLT; p++) {
> > +		freq_prev =3D freq;
> > +		freq =3D _calc_clk_freq(p);
> > +
> > +		if (req_freq >=3D freq)
> > +			break;
> > +	}
> > +
> > +	if (p =3D=3D PHASE_DFLT)
> > +		freq =3D DFLT_CLK_FREQ;
> > +
> > +	if (p =3D=3D PHASE_MIN || p =3D=3D PHASE_DFLT)
> > +		goto out;
> > +
> > +	/* Check which clock value from the identified range
> > +	 * is closer to the requested value
> > +	 */
> > +	if ((freq_prev - req_freq) < (req_freq - freq)) {
> > +		p =3D p - 1;
> > +		freq =3D freq_prev;
> > +	}
> > +out:
> > +	*phase =3D p;
> > +	*sample =3D _calc_sample(p);
> > +	return freq;
> > +}
> > +
> >  static int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
> >  				     const struct pci_device_id *ent)  { @@ -
> 59,6 +99,8 @@ static
> > int thunder_mdiobus_pci_probe(struct pci_dev *pdev,
> >  		struct mii_bus *mii_bus;
> >  		struct cavium_mdiobus *bus;
> >  		union cvmx_smix_en smi_en;
> > +		union cvmx_smix_clk smi_clk;
> > +		u32 req_clk_freq;
> >
> >  		/* If it is not an OF node we cannot handle it yet, so
> >  		 * exit the loop.
> > @@ -87,6 +129,29 @@ static int thunder_mdiobus_pci_probe(struct
> pci_dev *pdev,
> >  		bus->register_base =3D nexus->bar0 +
> >  			r.start - pci_resource_start(pdev, 0);
> >
> > +		smi_clk.u64 =3D oct_mdio_readq(bus->register_base +
> SMI_CLK);
> > +		smi_clk.s.clk_idle =3D 1;
> > +
> > +		if (!of_property_read_u32(node, "clock-freq",
> &req_clk_freq)) {
>=20
> Documentation/devicetree/bindings/net/mdio.yaml
>=20
> says:
>=20
>   clock-frequency:
>     description:
>       Desired MDIO bus clock frequency in Hz. Values greater than IEEE 80=
2.3
>       defined 2.5MHz should only be used when all devices on the bus supp=
ort
>       the given clock speed.
>=20
> Please use this property name, and update the binding for your device to
> indicate it is valid.
>=20

Yes, the property name and binding will be updated in V2 version.

> > +			u32 phase, sample;
> > +
> > +			dev_info(&pdev->dev, "requested bus clock
> frequency=3D%d\n",
> > +				 req_clk_freq);
>=20
> dev_dbg()
>=20
> > +
> > +			 bus->clk_freq =3D _config_clk(req_clk_freq,
> > +						     &phase, &sample);
>=20
> There should be some sort of range checks here, and return -EINVAL, if as=
ked
> to do lower/higher than what the hardware can support.
>=20

In _config_clk() function, the clock will be set to the nearest identified =
value to=20
the requested value.

> > +
> > +			 smi_clk.s.phase =3D phase;
> > +			 smi_clk.s.sample_hi =3D (sample >> 4) & 0x1f;
> > +			 smi_clk.s.sample =3D sample & 0xf;
>=20
> You indentation is messed up here. checkpatch would definitely of found
> that! Please do use checkpatch.
>=20
> > +		} else {
> > +			bus->clk_freq =3D DFLT_CLK_FREQ;
> > +		}
> > +
> > +		oct_mdio_writeq(smi_clk.u64, bus->register_base +
> SMI_CLK);
> > +		dev_info(&pdev->dev, "bus clock frequency set to %d\n",
> > +			 bus->clk_freq);
>=20
> Only use dev_info() for really important messages. We don't spam the kern=
el
> log for trivial things.
>=20
>        Andrew

All the rest of the comments will be taken care in V2 version.

Thanks,
Piyush