RE: [PATCH net-next 2/2] r8169: support RTL8168G

[hayeswang <hayeswang@realtek.com>]

[...]
> > +static void r8168_phy_ocp_write(void __iomem *ioaddr, u32 
> reg, u32 data)
> > +{
> > +	int i;
> > +
> > +	if (reg & 0xffff0001)
> > +		BUG();
> 
> The patch adds a lot of BUG(). BUG is terrible from a system 
> or end user
> viewpoint.
> 
> Were they only a devel helper or are they still supposed to be of use
> in the future ? If the latter applies, why ?
> 

I think if the reg is invalid, there must be something wrong. The code should
have bug, and I should notice develper or someone using the code. I would
replace them with showing messages.

[...]
> > +static void rtl_ocp_write_fw(struct rtl8169_private *tp, 
> struct rtl_fw *rtl_fw)
> > +{
> > +	struct rtl_fw_phy_action *pa = &rtl_fw->phy_action;
> > +	void __iomem *ioaddr = tp->mmio_addr;
> > +	u32 predata, count;
> > +	u32 base_addr;
> > +	size_t index;
> > +
> > +	predata = count = 0;
> > +	base_addr = 0xa400;
> > +
> > +	for (index = 0; index < pa->size; ) {
> > +		u32 action = le32_to_cpu(pa->code[index]);
> > +		u32 data = action & 0x0000ffff;
> > +		u32 regno = (action & 0x0fff0000) >> 16;
> > +
> > +		if (!action)
> > +			break;
> > +
> > +		switch(action & 0xf0000000) {
> > +		case PHY_READ:
> > +			predata = r8168_phy_ocp_read(ioaddr,
> > +					base_addr + (regno -16) * 2);
> > +			count++;
> > +			index++;
> > +			break;
> [duplicated code removed]
> > +		case PHY_WRITE:
> > +			if (regno == 0x1f)
> > +				base_addr = data << 4;
> > +			else
> > +				r8168_phy_ocp_write(ioaddr,
> > +						base_addr + 
> (regno - 0x10) * 2,
> > +						data);
> > +			index++;
> > +			break;
> [duplicated code removed]
> > +		case PHY_WRITE_PREVIOUS:
> > +			r8168_phy_ocp_write(ioaddr, base_addr + 
> (regno -16) * 2,
> > +					    predata);
> > +			index++;
> > +			break;
> 
> I can't believe that the hardware people have designed something which
> needs a different firmware write method, especially as it 
> copies at lot
> of code.
> 
> How did you come to the conclusion that it was not possible 
> to hide this
> stuff behind r8168g_mdio_{read / write} ?
> 
> I would not mind replacing the 
> PHY_{READ/WRITE/WRITE_PREVIOUS} case with
> chipset specific {READ/WRITE/WRITE_PREVIOUS} methods as long as the
> semantic looks the same but going through a different 
> (*write_fw) does not
> trivially seem to be the best abstraction.
> 

The difficulty is how to deal with the base_addr. Although it should not happen,
the base_addr may be modify if two threads access phy at the same time.  I would
replace the method by saving the base_addr with a global variable. Then, the
r8168g_mdio functions could deal with both of the firmware and phy settings.

[...]
> > +static void __devinit rtl_hw_initialize(struct rtl8169_private *tp)
> > +{
> > +	switch (tp->mac_version) {
> > +	case RTL_GIGA_MAC_VER_40:
> > +	case RTL_GIGA_MAC_VER_41:
> > +		rtl_hw_init_8168g(tp);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> 
> Why doesn't it belong to hw_start ?
> 

According to the initialization process from our hardware, there are something
needed to do before reset. Maybe this considers the rebooting from the other OS
or hwardware behavior.  I don't sure if it is safe, to let them belong to
hw_start.

> Is it completely unneeded if the device requires a rtl8169_hw_reset,
> resumes or such ?
>

By the information from our hardware, these are the initial settings and only
need be done once. 

Best Regards,
Hayes