RE: [PATCH 1/4] drivers: phy: add support for Armada CP110 UTMI PHY

[Kostya Porotchkin]

Hello, Lubomir,

Thank you for your review!

> > +static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port
> > +*port) {
> > +	u32 reg;
> > +
> > +	/*
> > +	 * Setup PLL. 40MHz clock used to be the default, being 25MHz now.
> > +	 * See the functional specification for details.
> 
> I tried to, couldn't find it. Perhaps you could elaborate more here, or include a
> link in the header.
[KP] This is the frequency of a quartz resonator connected to REFCLK_XIN/REFCLK_XOUT SoC pins.
The default crystal frequency used now at all Marvell reference platforms is 25MHz.
The default out of reset state register values are however have settings for 40MHz crystal used at the time of RTL code freeze.
I will mention this fact in the second patch version.

> 
> > +	/* PLL Power down if all UTMI PHYs are down */
> > +	regmap_clear_bits(utmi->syscon, SYSCON_USB_CFG_REG,
> > +USB_CFG_PLL_MASK);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mvebu_cp110_utmi_phy_power_on(struct phy *phy) {
> > +	struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
> > +	struct mvebu_cp110_utmi *utmi = port->priv;
> > +	struct device *dev = &phy->dev;
> > +	int ret;
> > +	u32 reg;
> > +
> > +	/* It is necessary to power off UTMI before configuration */
> > +	ret = mvebu_cp110_utmi_phy_power_off(phy);
> 
> mvebu_cp110_utmi_phy_power_off() also sometimes, but not always, shuts
> down the PLL. Is that necessary? I guess all you care about is the bit in
> UTMI_PHY_CFG_PU_MASK?
[KP] Not sure I fully understand the question. Both UTMI PHYs are using the same dedicated PLL source.
I am trying to save the power to shutting down this PLL when both PHY ports are down.

> 
> > +
> > +		ret = of_property_read_u32(child, "reg", &val);
> > +		if (ret < 0) {
> > +			dev_err(dev, "missing 'reg' property (%d)\n", ret);
> 
> A nit: the property is not necessarily missing -- it could be malformed (with ret
> of -ENODATA or -EOVERFLOW).
> 
> Also, perhaps you want to log the name of node that has problems ("%pOF",
> child); also in the error messages below.
[KP] OK, will do it in the second patch version. 

> 
> > +			continue;
> > +		}
> > +
> > +		if (val >= UTMI_PHY_PORTS) {
> > +			dev_err(dev, "invalid 'reg' property\n");
> > +			continue;
> > +		}
> 
> Perhaps you could just squelch those two warnings together:
> 
> 
> 	if (ret || val >= UTMI_PHY_PORTS) {
> 		dev_err(dev, "invalid 'reg' property on %pOF\n", child);
> 		continue;
> 	}
> 
[KP] Yes, it will definitely be better, thank you.

> > --
> > 2.17.1
> >
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__lists.infradead.org_mailman_listinfo_linux-2Darm-
> 2Dkernel&d=DwICAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=-
> N9sN4p5NSr0JGQoQ_2UCOgAqajG99W1EbSOww0WU8o&m=YFT4I6c34R7m8
> 6d4PlkWJFgC3qPXYkofB_VPJDgQVSA&s=Hf5wCKTcwKa3Mx-
> L7ZXEX4MPsfaMtPDu87RltnvXa90&e=