lkml.org 
[lkml]   [2012]   [Jul]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH net-next 2/2] r8169: support RTL8168G
    Date
    [...]
    > > +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



    \
     
     \ /
      Last update: 2012-07-02 10:02    [W:0.035 / U:121.980 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site