Prev: [PATCH 06/37] x86: call early_res_to_bootmem one time
Next: lockdep: Add information of file and line to lockdep_map
From: Ha, Tristram on 19 Jan 2010 16:30
Alan Cox wrote:
>> +/*
>> + * PCI Configuration Space Registers
>> + */
>
> We have existing standard defines for most of these that should be
used instead where relevant.
> The code appears not use most of these defines for generic PCI stuff
anyway.
>
>
>> +/*
>> + * Hardware register access macros
>> + */
>> +
>> +#define hw_dis_intr_sync(hw) hw_dis_intr(hw)
>> +
>> +#define HW_R8(hw, addr, data) (*(data) = readb((hw)->ioaddr +
(addr)))
>> +#define HW_W8(hw, addr, data) writeb((data), (hw)->ioaddr +
(addr)) +
>> +#define HW_R16(hw, addr, data) (*(data) = readw((hw)->ioaddr +
(addr)))
>> +#define HW_W16(hw, addr, data) writew((data), (hw)->ioaddr +
(addr)) +
>> +#define HW_R32(hw, addr, data) (*(data) = readl((hw)->ioaddr +
(addr)))
>> +#define HW_W32(hw, addr, data) writel((data), (hw)->ioaddr +
(addr)) +
>> +#define HW_PCI_READ_BYTE(hw, addr, data)
\
>> + pci_read_config_byte((struct pci_dev *) (hw)->pdev, addr, data)
+
>> +#define HW_PCI_READ_WORD(hw, addr, data)
\
>> + pci_read_config_word((struct pci_dev *) (hw)->pdev, addr, data)
+
>> +#define HW_PCI_READ_DWORD(hw, addr, data)
\
>> + pci_read_config_dword((struct pci_dev *) (hw)->pdev, addr, data)
+
>> +#define HW_PCI_WRITE_BYTE(hw, addr, data)
\
>> + pci_write_config_byte((struct pci_dev *) (hw)->pdev, addr, data)
+
>> +#define HW_PCI_WRITE_WORD(hw, addr, data)
\
>> + pci_write_config_word((struct pci_dev *) (hw)->pdev, addr, data)
+
>> +#define HW_PCI_WRITE_DWORD(hw, addr, data)
\
>> + pci_write_config_dword((struct pci_dev *) (hw)->pdev, addr,
data)
>
> This sort of stuff just obfuscates things so the defines ought to get
removed so the code is
> more readable to all (remember the Linux kernel code needs to be
generally readable not just
> readable by Micrel people)
>
>
>> + * delay_milli - delay in millisecond
>> + * @millisec: Number of milliseconds to delay.
>> + *
>> + * This routine delays in milliseconds.
>> + */
>> +static void delay_milli(uint millisec) {
>> + unsigned long ticks = millisec * HZ / 1000;
>> +
>> + if (!ticks || in_interrupt())
>> + mdelay(millisec);
>> + else {
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + schedule_timeout(ticks);
>
> msleep() is probably what you want for this part ? Note that you
really don't want to be
> spinning for milliseconds in the IRQ paths if at all possible.
>

The KSZ884X driver is modified from an old driver with shared code which
are used by other OS. I will clean it up further.


> The DEBUG ioctl is a bit odd - you define it and it does nothing -
just a left over ?
>
>

The KSZ884X has features and configurations that are not accessible from
standard Linux interface. The best way is probably using an application
to access the driver through ioctl interface. As that application
exists outside the kernel, I removed some code in the ioctl function. I
am still looking for a better solution to support those hardware
features. See my procfs comments below.


>
>> +
>> +#define PCI_VENDOR_ID_KS884X 0x16C6
>> +#define PCI_DEVICE_ID_KS8841 0x8841
>> +#define PCI_DEVICE_ID_KS8842 0x8842
>
> Those belong in the pci device id header.
>
>

I do not quite understand your suggestion. Do I need to put those IDs
in one of the kernel headers?


> In your pci init function you do the following
>
>> + hw->pdev = pdev;
>
>
> If you make a private copy of pdev in your struct you should refcount
it and use
> pci_dev_get/pci_dev_put when you take and release the reference.
>
>

The old driver uses shared code which compile in other OS. Those shared
functions use the hw structure, which does not use any OS dependent data
structures, as an anchor to access the hardware. In a few situations
the driver needs to write to the PCI configuration registers. That is
why the hw structure stores the pci device pointer as a generic pointer
variable.

I do not understand how pci_dev_get/pci_dev_put work. Does the pdev
pointer actually change during the lifetime of the PCI driver?


> The proc stuff probably belongs in sysfs nowdays

I need a quick way to access KSZ884X's hardware features without using a
user application. I use procfs as it is easy to implement. I am aware
that sysfs is preferred. But after trying it I do not think it is
appropriate for my purpose.

The sysfs interface requires just a filename, and read/write functions.
It does not provide a means to pass user-defined data. So it is fine to
just modify a simple global variable that controls a driver behavior.

The KSZ884X driver has same settings for each individual port. The
sysfs does not seem to have the capability to create an attribute with
filename like "port1/setting1," so an alternative is to use
"port1_setting1," "port1_setting2," and so on. It does not look
organized. (I used the DEVICE_ATTR macro, and the compiler complained
about the name. I probably did it wrong.) Also, as the attribute
access functions do not know anything about the port, the same identical
functions have to be created for each port. It is not efficient.

I also like the attribute to associate with the network device rather
than the PCI device, as the KSZ8842 driver can create another virtual
network device. I tried to pass the device pointer of the network
device to device_create_file function and the driver crashed. I will
investigate this matter further.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Alan Cox on 19 Jan 2010 16:40
> >> +#define PCI_VENDOR_ID_KS884X 0x16C6
> >> +#define PCI_DEVICE_ID_KS8841 0x8841
> >> +#define PCI_DEVICE_ID_KS8842 0x8842
> >
> > Those belong in the pci device id header.
> >
> >
>
> I do not quite understand your suggestion. Do I need to put those IDs
> in one of the kernel headers?

Into include/linux/pci_ids.h

> >> + hw->pdev = pdev;
> >
> >
> > If you make a private copy of pdev in your struct you should refcount
> it and use
> > pci_dev_get/pci_dev_put when you take and release the reference.

> I do not understand how pci_dev_get/pci_dev_put work. Does the pdev
> pointer actually change during the lifetime of the PCI driver?

No but it can go away if the device is removed. The pci_dev_get ensures
it won't go away while you have a pointer to it. and the pci_dev_put
gives up your reference.

> I also like the attribute to associate with the network device rather
> than the PCI device, as the KSZ8842 driver can create another virtual
> network device. I tried to pass the device pointer of the network
> device to device_create_file function and the driver crashed. I will
> investigate this matter further.

Ok
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Stephen Hemminger on 19 Jan 2010 16:50
On Tue, 19 Jan 2010 12:03:20 -0800
"Ha, Tristram" <Tristram.Ha(a)Micrel.Com> wrote:

> David Miller wrote:
> > From: "Ha, Tristram" <Tristram.Ha(a)Micrel.Com>
> > Date: Fri, 15 Jan 2010 18:57:59 -0800
> >
> >> The KSZ8842 has a switch with lots of hardware configurations. The =
> >> driver uses the proc system to allow users to configure the switch.
> >> If = this is not desired the whole thing can be removed by not
> calling the =
> >> init_proc() function.
> >
> > I think there needs to be a serious discussion about how this driver
> uses bridge layer internals
> > by doing things like:
> >
> > +/* Needed for STP support. */
> > +#ifdef CONFIG_KSZ8842_STP
> > +#include <../net/bridge/br_private.h>
> > +#endif
> >
> > and uses procfs to configure the ports.
> >
> > Stephen please look this over and make suggestions for better ways to
> support and configure
> > these kinds of devices.
> >
> > Thanks.
>
> I like to explain a little bit about this Spanning Tree Protocol
> support.
>
> Micrel KSZ8842's 3-port switch and Micrel's other 5-port switches have
> port controls to enable/disable tx and rx and stop MAC address learning.
> They are supposed to help run STP more efficiently, but somebody needs
> to control those ports.
>
> From my observation of how the brctl application controls the network
> devices when running STP, I know the kernel bridge puts all the devices
> under it in promiscuous mode and declares the state of each bridge port
> associated with the device blocked or forwarding depending on the BPDU
> frames received. When the port is blocked, the device is still active
> and passes all frames to the host. The bridge only looks at BPDU frames
> and drops all other frames. It is better to just shut off the port.

Ok.

> From the time when the KSZ8842 driver was developed for the Linux 2.4
> kernel I looked for a kernel API to tell the bridge port's state so the
> device driver can shut off the port if necessary. I could not find one
> and so I came up with this hack to look at the bridge port's structure
> directly. It looks dangerous but is quite safe. The driver only looks
> at the bridge port state variable and finds out the MAC address
> associated with the bridge device. It can get the state definitions
> from the if_bridge header. The private bridge structure may change in
> the future and break the code, but as kernel network interfaces are
> changing all the time, the driver just needs to be modified for the new
> version. To avoid this situation, the kernel may need to export two
> functions to tell the bridge port state and bridge device address and
> put the prototypes in the if_bridge header.

There was one added for user level RSTP support.
RSTP is where Linux STP is headed, so adding more hooks into
existing STP is going backwards.

> Now for the driver implementation for STP support. I programmed the
> switch's static MAC table to always pass the following frames to the
> host: BPDU frames with specific multicast address, broadcast frames,
> unicast frames with the device bridge's MAC address, and multicast
> frames with ICMPv6 multicast address. All other frames are not passed
> to the host and are handled by the switch, forwarding each frame with
> its standard forwarding logic. The port can be shut off if it is
> blocked and those frames will not pass through that port. The host gets
> BPDU frames so that the bridge can determine each port's state. The
> other broadcast, unicast, and multicast frames passed to the host are
> necessary if some other network devices want to communicate with the
> host. As the forwarding is done by hardware rather than software,
> overall performance does increase.

What about LACP needed by bridging?

> I did verify the driver disables or enables the port appropriately when
> the bridge port state changes, but as I do not have the experience of
> running a full-scale Spanning Tree, I do not know if this hardware
> implementation behaves the same as the software one provided by the
> kernel and brctl. I also did not try using VLAN.


--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ha, Tristram on 19 Jan 2010 17:00
>> If my assumptions are correct, then I thing it would be better to have
>> a driver that presents this device as two: one - an ethernet device,
>> and second - a switch management interface like the one used in OpenWRT.
>>
>> Here are some links for reference:
>> http://downloads.openwrt.org/kamikaze/8.09.2/kamikaze_8.09.2_source.ta r.bz2;
>> subdir: package/switch/src/
>> -> switch management driver for ADMTEK Adm6996 and Broadcom
>> BCM5325E/536x
> Don't look at that ;)
> The code you're referring to is just a simple hack of mine that is a few years old now. It's
> going to be removed as soon as the drivers have been ported to a proper API.
>
> I've been working on a switch configuration API based on netlink, which is being used on some of
> the other platforms in OpenWrt.
>
> You can find the sources here:
> https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/drivers/net/phy/swconfig.c
> https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/include/linux/switch.h
>
> It's meant to be used with drivers that hook into the PHY abstraction layer.
> You can find an example of such a driver here:
> https://dev.openwrt.org/export/19173/trunk/target/linux/generic-2.6/files/drivers/net/phy/rtl8306.c
>
> I'll do the final cleanups and submit it for review once I have time to do so.
>
> - Felix

Your work looks interesting, but rtl8036 seems to use MDIO page registers to access its own internal registers. Micrel switches do not have that feature and so cannot use that way.

I am looking at those PHY drivers and try to figure out how they work under mdio bus. I am using Micrel's own KSZ8695P and KSZ9692P SoC to develop and debug the KSZ884X PCI driver. KSZ9692P actually has two external PHYs so that the PHY driver can run on it. But the driver does not load because I probably need to add some hooks to the mdio bus driver to do the actual hardware access. Can somebody give me some pointers to this mdio bus support? Does the PHY driver create a user interface like eth0 so that it can be accessed using some tools like ethtool?

Anyway I am afraid the phy driver model does not work on KSZ884X. I cannot think of how the mdio bus accesses hardware registers located on a PCI device.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Felix Fietkau on 19 Jan 2010 17:20
On 2010-01-19 10:51 PM, Ha, Tristram wrote:
> Your work looks interesting, but rtl8036 seems to use MDIO page
> registers to access its own internal registers. Micrel switches do
> not have that feature and so cannot use that way.
>
> I am looking at those PHY drivers and try to figure out how they work
> under mdio bus. I am using Micrel's own KSZ8695P and KSZ9692P SoC to
> develop and debug the KSZ884X PCI driver. KSZ9692P actually has two
> external PHYs so that the PHY driver can run on it. But the driver
> does not load because I probably need to add some hooks to the mdio
> bus driver to do the actual hardware access. Can somebody give me
> some pointers to this mdio bus support? Does the PHY driver create a
> user interface like eth0 so that it can be accessed using some tools
> like ethtool?
>
> Anyway I am afraid the phy driver model does not work on KSZ884X. I
> cannot think of how the mdio bus accesses hardware registers located
> on a PCI device.
The switch config core that i wrote is not specific to PHY drivers, it
can be used from ethernet drivers, GPIO based platform drivers or any
other kind of drivers as well.
It exports a simple netlink based config interface to user space that
allows you to get a list of all configurable attributes for the device,
ports and vlans, and change them.

I also have a config utility for it:
https://dev.openwrt.org/browser/trunk/package/swconfig/src

It currently depends on our config library (libuci), but that can be
removed easily, as the code for that is not spread over the core of the
utility.

Part of this code can also be used as a small library, if you want to
write your own utility.

- Felix
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: [PATCH 06/37] x86: call early_res_to_bootmem one time
Next: lockdep: Add information of file and line to lockdep_map