Messages in this thread Patch in this message |  | | From | Piyush Malgujar <> | | Subject | Re: [PATCH] Marvell MDIO clock related changes. | | Date | Fri, 29 Apr 2022 14:14:00 +0000 |
| |
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@marvell.com> > > Signed-off-by: Damian Eppel <deppel@marvell.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
|  |