Date:	Tue, 29 Dec 2015 08:18:17 +0000
From:	Shaohui Xie <shaohui.xie@....com>
To:	Florian Fainelli <f.fainelli@...il.com>,
	"shh.xie@...il.com" <shh.xie@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>
CC:	"Shaohui.Xie@...escale.com" <Shaohui.Xie@...escale.com>
Subject: RE: [PATCH] net: phy: adds backplane driver for Freescale's PCS PHY

Hello Florian,

Thanks for reviewing the patch! 

> > +struct training_state_machine {
> > +	bool bin_m1_late_early;
> > +	bool bin_long_late_early;
> > +	bool bin_m1_stop;
> > +	bool bin_long_stop;
> > +	bool tx_complete;
> > +	bool an_ok;
> > +	bool link_up;
> > +	bool running;
> > +	bool sent_init;
> > +	int m1_min_max_cnt;
> > +	int long_min_max_cnt;
> 
> Can you change this into a succession of enum values to make it clear how
> many states there are, and how to get from one to the other?
[S.H] OK. I'll make the states clear in next version. 
Actually, the structure members above are not all states, most of them are 
conditions used to decide what actions should be taken during training.

> 
> [snip]
> 
> > +
> > +static void init_state_machine(struct training_state_machine *s_m) {
> > +	s_m->bin_m1_late_early = true;
> > +	s_m->bin_long_late_early = false;
> > +	s_m->bin_m1_stop = false;
> > +	s_m->bin_long_stop = false;
> > +	s_m->tx_complete = false;
> > +	s_m->an_ok = false;
> > +	s_m->link_up = false;
> > +	s_m->running = false;
> > +	s_m->sent_init = false;
> > +	s_m->m1_min_max_cnt = 0;
> > +	s_m->long_min_max_cnt = 0;
> 
> That alone is a good indication that your state machine is not readable.
[S.H] Will improve it.

> 
> > +}
> > +
> > +void tune_tecr0(struct fsl_xgkr_inst *inst) {
> > +	struct per_lane_ctrl_status *reg_base;
> > +	u32 val;
> > +
> > +	reg_base = (struct per_lane_ctrl_status *)inst->reg_base;
> 
> Typical practice is not to cast a register base address into a pointer to
> a structure, but instead use offsets to the base register address, which
> is less error prone and more readable, anything that uses
> inst->reg_base in this driver is modeled after a structure pattern,
> please fix that.
[S.H] OK. The inst->reg_base can be used directly without cast since it's
defined as void*, will drop the cast.

> 
> > +
> > +	val = TECR0_INIT |
> > +		inst->adpt_eq << ZERO_COE_SHIFT |
> > +		inst->ratio_preq << PRE_COE_SHIFT |
> > +		inst->ratio_pst1q << POST_COE_SHIFT;
> > +
> > +	/* reset the lane */
> > +	iowrite32be(ioread32be(&reg_base->gcr0) & ~GCR0_RESET_MASK,
> > +		    &reg_base->gcr0);
> > +	udelay(1);
> > +	iowrite32be(val, &reg_base->tecr0);
> > +	udelay(1);
> > +	/* unreset the lane */
> > +	iowrite32be(ioread32be(&reg_base->gcr0) | GCR0_RESET_MASK,
> > +		    &reg_base->gcr0);
> > +	udelay(1);
> 
> Since this is a reset control register, you might want to explore using
> the reset controller subsystem in case this register serves multiple
> peripherals.
[S.H] this is per lane register, the lane cannot serve multiple peripherals 
at a time, when it serves backplane, it only serves backplane.

> 
> [snip]
> 
> I skipped through all the state machine logic, but you should consider
> trying to simplify it using different enum values and a switch() case()
> statements to make it easy to audit and understand.
[S.H] OK. I'll make state machine logic clear.

> 
> > +
> > +static int fsl_backplane_probe(struct phy_device *phydev) {
> > +	struct fsl_xgkr_inst *xgkr_inst;
> > +	struct device_node *child, *parent, *lane_node;
> > +	const char *lane_name;
> > +	int len;
> > +	int ret;
> > +	u32 mode;
> > +	u32 lane[2];
> > +
> > +	child = phydev->dev.of_node;
> > +	parent = of_get_parent(child);
> > +	if (!parent) {
> > +		dev_err(&phydev->dev, "could not get parent node");
> > +		return 0;
> > +	}
> > +
> > +	lane_name = of_get_property(parent, "lane-instance", &len);
> > +	if (!lane_name)
> > +		return 0;
> > +
> > +	if (strstr(lane_name, "1000BASE-KX"))
> > +		mode = BACKPLANE_1G_KX;
> > +	else
> > +		mode = BACKPLANE_10G_KR;
> 
> That seems like I could put whatever value in "lane-instance" and as long
> as it contains 1000BASE-KX we are golden, that does not sound very robust
> nor well defined.
[S.H] Will use strcmp instead of strstr, also add check for 10GBASE-KR.

> 
> > +
> > +	lane_node = of_parse_phandle(child, "lane-handle", 0);
> > +	if (lane_node) {
> 
> Treat this as an error so you can return early and reduce indentation.
[S.H] Ok. Will use if (!lane_node).

> 
> > +		struct resource res_lane;
> > +
> > +		ret = of_address_to_resource(lane_node, 0, &res_lane);
> > +		if (ret) {
> > +			dev_err(&phydev->dev, "could not obtain memory map\n");
> > +			return ret;
> > +		}
> > +
> > +		ret = of_property_read_u32_array(child, "lane-range",
> > +						 (u32 *)&lane, 2);
> > +		if (ret) {
> > +			dev_info(&phydev->dev, "could not get lane-range\n");
> > +			return -EINVAL;
> > +		}
> 
> Is not the standard "ranges" property usable here?
[S.H] "lane-range" defines offset and length of register set for a serdes lane,
The standard "ranges" property might not quite fit here.

> 
> > +
> > +		phydev->priv = devm_ioremap_nocache(&phydev->dev,
> > +						    res_lane.start + lane[0],
> > +						    lane[1]);
> 
> What about the "reg" property here?
[S.H] "reg" property is PHY address.

> 
> > +
> > +		if (!phydev->priv) {
> > +			of_node_put(lane_node);
> > +			return -EINVAL;
> > +		}
> > +		of_node_put(lane_node);
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (mode == BACKPLANE_1G_KX) {
> > +		phydev->speed = SPEED_1000;
> > +		/* configure the lane for 1000BASE-KX */
> > +		lane_set_1gkx(phydev->priv);
> > +		return 0;
> > +	}
> > +
> > +	xgkr_inst = kzalloc(sizeof(*xgkr_inst), GFP_KERNEL);
> > +	if (!xgkr_inst)
> > +		goto mem_err1;
> 
> devm_kzalloc()?
[S.H] Will use this.

> 
> > +
> > +	xgkr_inst->reg_base = phydev->priv;
> > +	xgkr_inst->bus = phydev->bus;
> > +	xgkr_inst->phydev = phydev;
> > +	phydev->priv = xgkr_inst;
> > +
> > +	if (mode == BACKPLANE_10G_KR) {
> > +		phydev->speed = SPEED_10000;
> > +		init_inst(xgkr_inst, 1);
> > +		INIT_DELAYED_WORK(&xgkr_inst->xgkr_wk, xgkr_wq_state_machine);
> > +	}
> > +
> > +	return 0;
> > +mem_err1:
> > +	dev_err(&phydev->dev, "failed to allocate memory\n");
> > +	return -ENOMEM;
> > +}
> > +
> > +static int fsl_backplane_aneg_done(struct phy_device *phydev) {
> > +	return 1;
> > +}
> > +
> > +static int fsl_backplane_config_aneg(struct phy_device *phydev) {
> > +	struct fsl_xgkr_inst *xgkr_inst = (struct fsl_xgkr_inst
> > +*)phydev->priv;
> 
> No casting needed from void *
[S.H] OK.

> 
> [snip].
> 
> > +
> > +static int fsl_backplane_read_status(struct phy_device *phydev) {
> > +	int val;
> > +
> > +	phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > +	val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > +
> > +	if (val & AN_LNK_UP_MASK)
> > +		phydev->link = 1;
> > +	else
> > +		phydev->link = 0;
> 
> This sounds fairly generic, candidate for a helper function?
[S.H] OK. Will make it a helper function.

Thank you!
Shaohui

Powered by blists - more mailing lists