Re: [PATCH] Marvell MDIO clock related changes.

[Andrew Lunn]

On Fri, Apr 15, 2022 at 07:30:26AM -0700, Piyush Malgujar wrote:
> 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.

> 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.

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

> 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@xxxxxxxxxxx>
> Signed-off-by: Damian Eppel <deppel@xxxxxxxxxxx>

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..ed4c48d8a38bd80e6a169f7a6d90c1f2a0daccfc 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..642d08aff3f7f849102992a891790e900b111d5c 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.

> +			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.

> +
> +			 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