lkml.org 
[lkml]   [2011]   [Aug]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] Add pch ieee1588 driver for Intel EG20T PCH
    Date
    Hi Richard

    I added comment below.
    Please confirm them

    > +/* Register read/write macros */
    > +#define PCH_BIT_SET_CHECK(addr, bitmask) \
    > + ((ioread32(addr) & (bitmask)) == (bitmask))
    > +#define PCH_SET_ADDR_BIT(addr, bitmask)\
    > + iowrite32((ioread32(addr) | (bitmask)), (addr))
    > +#define PCH_CLR_ADDR_BIT(addr, bitmask)\
    > + iowrite32((ioread32(addr) & ~(bitmask)), (addr))
    >
    >I don't really like macros of this sort, just to set and clear
    >register bits. It doesn't make any fewer lines nor does it clarify the
    >code. Every driver author will know what is meant by the plain old C,
    >like:
    >
    > x = x | mask;
    > x = x & ~mask;
    >
    >Also, you should consider whether you need to protect against
    >concurrent access on a register.

    modify

    >> +
    >> +#define DEFAULT_ADDEND 0xF0000029
    >> +#define TICKS_NS_SHIFT 4
    >
    >This driver is based on my ixp46x driver, which is fine because,
    >looking at the data sheet, it appears that the time stamping unit in
    >the EG20T PCH is fairly similar.
    >
    >However, I doubt that these ADDEND and SHIFT values will work for you,
    >since they were calculated for the frequency compensated clock on the
    >IXP. They reflect an input clock frequency of 66.666666 MHz and a
    >nominal frequency of 62.5 MHz (or a period of 16 nanoseconds, thus
    >SHIFT is 4).
    >
    >Section 14.6.1.10 in the data sheet seems to imply that the input
    >clock is 50 MHz. In that case you could use a nominal frequency of
    >31.25 MHz (period 32 ns, SHIFT 5). However, you need to find out
    >the actual input clock frequency. If this can vary, then the driver
    >should allow changing these values.

    modify as follows.

    #define DEFAULT_ADDEND 0xA0000000
    #define TICKS_NS_SHIFT 5


    >> + u32 asms_hi;
    >> + u32 amms_lo;
    >> + u32 amms_hi;
    >> + u32 ch_control;
    >
    >You never program this register. But I think the "Mode" field should
    >be set to 2. The other settings really make no sense at all. Or do
    >you set this in the MAC driver?
    >
    >(I wonder why Intel retained the very limited PTP V1 modes from the
    >IXP time stamping unit.)

    ch_control register was set as 0x80020000 (Version:1, mode:2 )


    >> + u32 mem_base;
    >> + u32 mem_size;
    >> + u32 irq;
    >> + u32 suspend:1;
    >> + u32 initialized:1;
    >> + struct pci_dev *pdev;
    >> + spinlock_t lock;
    >
    >It would be nice to have a short comment telling what this lock
    >protects. (Yes, I know what it protects, but still that is good
    >practice to have a comment.)

    It renames to "register_lock"

    >> +static inline void pch_eth_enable_set(struct pch_dev *chip)
    >> +{
    >> + /* SET the eth_enable bit */
    >> + PCH_SET_ADDR_BIT(&chip->regs->ts_sel, PCH_ECS_ETH);
    >> +}
    >
    >I really don't like this or the other similar helper functions,
    >below. They don't make the driver more understandable or shorter.
    >This function is only used in one place. You need at least two callers
    >to justify this.

    modify

    >I think the following helper functions ...
    >
    >> +static inline u32 pch_pps_evt_get(struct pch_dev *chip)
    >> +{
    >> + /* Poll for PPS event */
    >> + return PCH_BIT_SET_CHECK(&chip->regs->event, PCH_TSE_PPS);
    >> +}
    :
    :
    >> +
    >> +static inline void pch_ttm_evt_clear(struct pch_dev *chip)
    >> +{
    >> + /* Clear Target Time Reached event */
    >> + PCH_SET_ADDR_BIT(&chip->regs->event, PCH_TSE_TTIPEND);
    >> +}
    >
    >... are just noise and add nothing useful to the driver.

    These functions are removed.

    >> +static void pch_reset(struct pch_dev *chip)
    >> +{
    >> + /* Reset Hardware Assist */
    >> + pch_block_reset(chip);
    >> +
    >> + /* enable all 32 bits in system time registers */
    >> + pch_set_system_time_count(chip);
    >> +}
    >
    >These three, above, are okay, since they encapsulate one logical
    >operation that takes more than one register access.
    >
    >However, you might consider whether you need locking here.

    add spin lock.

    >> +static void pch_eth_enable(struct pch_dev *chip)
    >> +{
    >> + pch_eth_enable_set(chip);
    >> +}
    >
    >Again, this helper only has one caller. Why not just set the bit that
    >you need in line?

    modify

    > +
    > +/*
    > + * Interrupt service routine
    > + */
    > +static irqreturn_t isr(int irq, void *priv)
    > +{
    > + struct pch_dev *pch_dev = priv;
    > + struct pch_ts_regs *regs = pch_dev->regs;
    > + struct ptp_clock_event event;
    > + u32 ack = 0, lo, hi, val;
    > +
    > + val = ioread32(&regs->event);
    > +
    > + if (val & PCH_TSE_SNS) {
    > + ack |= PCH_TSE_SNS;
    > + if (pch_dev->exts0_enabled) {
    > + hi = ioread32(&regs->asms_hi);
    > + lo = ioread32(&regs->asms_lo);
    > + event.type = PTP_CLOCK_EXTTS;
    > + event.index = 0;
    > + event.timestamp = ((u64) hi) << 32;
    > + event.timestamp |= lo;
    > + event.timestamp <<= TICKS_NS_SHIFT;
    > + ptp_clock_event(pch_dev->ptp_clock, &event);
    > + }
    > + }
    > +
    > + if (val & PCH_TSE_SNM) {
    > + ack |= PCH_TSE_SNM;
    > + if (pch_dev->exts1_enabled) {
    > + hi = ioread32(&regs->amms_hi);
    > + lo = ioread32(&regs->amms_lo);
    > + event.type = PTP_CLOCK_EXTTS;
    > + event.index = 1;
    > + event.timestamp = ((u64) hi) << 32;
    > + event.timestamp |= lo;
    > + event.timestamp <<= TICKS_NS_SHIFT;
    > + ptp_clock_event(pch_dev->ptp_clock, &event);
    > + }
    > + }
    > +
    > + if (val & PCH_TSE_TTIPEND)
    > + ack |= PCH_TSE_TTIPEND; /* this bit seems to be always set */
    >
    >This ISR code (and much of the rest of the driver) is copied from the
    >IXP driver. It would be nice to know if it actually works on the atom
    >hardware. Do have any hardware to try it on?

    We have a board of atom(E600) and EG20T.
    However there is no environment for confirming auxiliary snapshot.

    >> +static struct ptp_clock_info ptp_pch_caps = {
    >> + .owner = THIS_MODULE,
    >> + .name = "PCH timer",
    >> + .max_adj = 66666655,
    >
    >This should be recalculated once you figure out the input clock and
    >nominal frequency.

    modify to .max_adj = 50000000,

    >> +#ifdef CONFIG_PM
    >> +static s32 pch_suspend(struct pci_dev *pdev, pm_message_t state)
    >> +{
    >> + struct pch_dev *chip = pci_get_drvdata(pdev);
    >> +
    >> + chip->suspend = 1;
    >
    >You set this flag here ...

    remove

    >> + /* allocate the memory for the device registers */
    >> + if (!request_mem_region
    >> + (chip->mem_base, chip->mem_size, "1588_regs")) {
    >
    >Poor statement break (and this would fit all on one line).

    modify

    > + dev_err(&pdev->dev,
    > + "%s: could not allocate register memory space\n", __func__);
    >
    >Bad indentation.

    modify

    >> + if (ret != 0) {
    >> + dev_err(&pdev->dev,
    >> + "%s: failed to get irq %d\n", __func__, pdev->irq);
    >> + goto err_req_irq;
    >> + }
    >> +
    >> + chip->initialized = 1;
    >
    >You set this flag, but never use it.

    remove

    >> +
    >> +static DEFINE_PCI_DEVICE_TABLE(pch_gbe_pcidev_id) = {
    >> + {.vendor = PCI_VENDOR_ID_INTEL,
    >
    >Needs a space (or newline) before the dot.

    modify

    >> +
    >> +static struct pci_driver pch_pcidev = {
    >> + .name = KBUILD_MODNAME,
    >> + .id_table = pch_pcidev_id,
    >
    >Here you meant "pch_gbe_pcidev_id" instead (or no "gbe" in the PCI
    >device table).

    modify

    >Overall, the driver looks okay. I would appreciate if you would take a
    >look at the comments and submit a revised patch.
    >
    >I would also like to see how the time stamps are done in the MAC
    >driver. Have you already posted that?
    >

    Mac driver already upstreamed. ( drivers/net/pch_gbe)
    However the code relevant to IEEE1588 is not contained into Ethernet driver.
    I have question about rx/tx snapshot.
    Is it necessary to use a snapshot within an Ethernet driver driver?
    I was indicated that there must not be any dependence of a device when the
    Ethernet driver was upstreamed.

    >Feature request: I notice in the data sheet that the time stamping
    >unit can produce a PPS output. Any chance that you could program this
    >feature?

    Currently, there is no plan.

    Best Regards
    Toshiharu Okada



    \
     
     \ /
      Last update: 2011-08-10 07:13    [W:0.040 / U:183.208 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site