Date:   Fri, 29 Apr 2022 14:14:00 +0000
From:   Piyush Malgujar <pmalgujar@...vell.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Chandrakala Chavva <cchavva@...vell.com>,
        Damian Eppel <deppel@...vell.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH] Marvell MDIO clock related changes.

Hi Andrew,

Firstly, thanks for your time to review this patch.

> > This patch includes following support related to MDIO Clock:
> 
> 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.
> 
> git log --oneline mdio-thunder.c will give you an idea what to use.
> 

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).
> 
> Please read 802.3 Clause 22. The default should be 2.5MHz.
> 

These changes are only specific to Marvell Octeon family.

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

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 will
> 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
> 
> Please keep line lengths to 80. I'm surprised checkpatch did not warn about
> this.
> 
> 
> > 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@...vell.com>
> > Signed-off-by: Damian Eppel <deppel@...vell.com>
> 
> These are in the wrong order. Since you are submitting it, your
> Signed-off-by: comes last.
> 
> > ---
> >  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)
> 
> Please avoid macros like this. Use a function.
> 
> > +
> > +#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 = 0, freq_prev;
> > +
> > +	for (p = PHASE_MIN; p < PHASE_DFLT; p++) {
> > +		freq_prev = freq;
> > +		freq = _calc_clk_freq(p);
> > +
> > +		if (req_freq >= freq)
> > +			break;
> > +	}
> > +
> > +	if (p == PHASE_DFLT)
> > +		freq = DFLT_CLK_FREQ;
> > +
> > +	if (p == PHASE_MIN || p == 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 = p - 1;
> > +		freq = freq_prev;
> > +	}
> > +out:
> > +	*phase = p;
> > +	*sample = _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 = nexus->bar0 +
> >  			r.start - pci_resource_start(pdev, 0);
> >
> > +		smi_clk.u64 = oct_mdio_readq(bus->register_base +
> SMI_CLK);
> > +		smi_clk.s.clk_idle = 1;
> > +
> > +		if (!of_property_read_u32(node, "clock-freq",
> &req_clk_freq)) {
> 
> Documentation/devicetree/bindings/net/mdio.yaml
> 
> says:
> 
>   clock-frequency:
>     description:
>       Desired MDIO bus clock frequency in Hz. Values greater than IEEE 802.3
>       defined 2.5MHz should only be used when all devices on the bus support
>       the given clock speed.
> 
> Please use this property name, and update the binding for your device to
> indicate it is valid.
> 

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

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

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

> > +
> > +			 smi_clk.s.phase = phase;
> > +			 smi_clk.s.sample_hi = (sample >> 4) & 0x1f;
> > +			 smi_clk.s.sample = sample & 0xf;
> 
> You indentation is messed up here. checkpatch would definitely of found
> that! Please do use checkpatch.
> 
> > +		} else {
> > +			bus->clk_freq = 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);
> 
> Only use dev_info() for really important messages. We don't spam the kernel
> log for trivial things.
> 
>        Andrew

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

Thanks,
Piyush

Powered by blists - more mailing lists