Ira W. Snyder wrote:
> On Mon, Feb 15, 2010 at 06:13:11PM +0100, Wolfgang Grandegger wrote:
>> Hi Ira,
>>
>> Ira W. Snyder wrote:
>>> +/* Get the MSYNC bits from the "old-style" interface control registers */
>>> +static void janz_get_msync(struct janz_ican3 *mod, u8 *locl, u8 *peer)
>>> +{
>>> + janz_set_page(mod, 0);
>>> + *peer = ioread8(mod->regs + MSYNC_PEER);
>>> + *locl = ioread8(mod->regs + MSYNC_LOCL);
>>> +}
>> What are your arguments against using structures to describe the
>> register layout?
>>
>
> I don't have any strong arguments against using a structure to describe
> register layout, except:
>
> 1) all datasheets I've ever used have registers described by hexadecimal
> offsets. Using a structure, you have to manually calculate offsets when
> checking that the structure is defined correctly. Once it is correct,
> this isn't a big deal.
In general structs are preferred as they allow type checking and result
usually in more readable code. #defines are more handy if you deal with
offset instead of addresses.
> 2) These registers only exist in a single DPM page, page #0. They are
> not "always accessible", as in a device which uses mmio. You must switch
> the DPM page before accessing them.
>
> Does it really make sense to create a structure just for calculating the
> offsets in one DPM page? For the "janz onboard registers" (described
> with "JANZ_OB_* defines), I agree. I'll make a structure for those.
Please check if it results in better code.
[snip]
>>> + if ((xord & 0x30) == 0x30) {
>>> + dev_err(mod->dev, "no mbox for writing\n");
>>> + return -ENOMEM;
>>> + }
>> Here and in many other places macro definitions would make the code more
>> readable.
>>
>
> Yep, There are a few of these. My problem is this: what names do I use?
> The way this device works is stupid. You'll want to read pages C-333 to
> C-335 in the manual.
>
> The basics are:
> You have two registers, MSYNC_PEER and MSYNC_LOCL. The firmware can
> only write to MSYNC_PEER, and the driver can only write to MSYNC_LOCL.
>
> The registers are laid out like this:
>
> MSYNC_PEER:
> bit0: RB0
> bit1: RB1
> bit2: LW
> bit4: WB0
> bit5: WB1
>
> MSYNC_LOCL:
> bit0: RB0
> bit1: RB1
> bit4: WB0
> bit5: WB1
> bit6: LW
>
> When you get an interrupt, and you want to read a message from the
> controller, you must XOR the RBn bits. If you get a 1 back for a certain
> bit position, then you have a buffer to read. The same is true for the
> WBn bits, except that the XOR will be 0 when you have a buffer to read.
>
> In addition, you must read/set the LW (last written) bit to correspond
> to the buffer you last wrote to.
>
> I would be happy to use defines to make this easier to read, but I have
> *no idea* what to name them. Suggestions please.
#define MSYNC_PEER_RB0 0x01
#define MSYNC_PEER_RB1 0x02
and so on.
BTW: an inline function for that bit gymnastics would be useful as well.
[snip]
>>> + memset(&msg, 0, sizeof(msg));
>>> + msg.control = 0x00;
>>> + msg.spec = MSG_INITFDPMQUEUE;
>>> + msg.len = cpu_to_le16(8);
>> Please do not align expressions. Just use *one* space before and after
>> "=". Please fix globally.
>>
>
> Ok. This will be in the next version of the patch. I thought it made the
> code easier to read.
It's required by the coding style.
>>> +static void can_to_janz(struct janz_ican3 *mod, struct can_frame *cf,
>>> + struct janz_fast_desc *desc)
>> Use a better name please.
>>
>
> I'll try to think something up. All these two functions do is convert a
> struct can_frame to the appropriate structure for the firmware, and
> back. How about "janz_convert_tocan() and janz_convert_fromcan()"?
can_frame_to_janz and janz_to_can_frame would already be fine. "can" is
to general.
[snip]
>>> +
>>> + cf->can_id |= idflags;
>>> + cf->data[1] = d1;
>> Hm, the data field to be used depends on the error type.
>>
>
> This code is copied from esd_pci331.c. If I don't have a good example,
> how can I be expected to do things correctly!
>
> How do I find out exactly which field belongs to which data type?
See include/linux/can/error.h
>>> +
>>> + netif_rx(skb);
>>> +
>>> + stats->rx_packets++;
>>> + stats->rx_bytes += cf->can_dlc;
>>> + return 0;
>>> +}
[snip]
>>> + rxerr = msg->data[4];
>>> + txerr = msg->data[5];
>> Should go to field 6 and 7.
>>
>
> You're confused. These come from the firmware, they are not going into a
> struct can_frame. These are the buffer locations in the message sent by
> the firmware for rx/tx error counter registers.
Right. Sorry for the noise.
>>> +
>>> + /* state is error-active by default */
>>> + state = CAN_STATE_ERROR_ACTIVE;
>>> +
>>> + if (rxerr >= 96 || txerr >= 96)
>>> + state = CAN_STATE_ERROR_WARNING;
>>> +
>>> + if (rxerr >= 128 || txerr >= 128)
>>> + state = CAN_STATE_ERROR_PASSIVE;
>>> +
>>> + if (rxerr >= 255 || txerr >= 255)
>>> + state = CAN_STATE_BUS_OFF;
>> You could use "else if" if you revert the order.
>
> Ok, I'll try that in the next version.
>
>> Also, 255 does not yet
>> mean bus-error, strictly speaking. Have you seen the device going bus-off?
>>
>
> Well, these are byte-sized registers. They can never exceed 255 (0xff),
> they would exceed their bit-width. What *should* I do here?
>
> I don't think I've seen the device go bus-off. How would I cause that
> situation?
Short-circuit the CAN low and high wires and send a message. Do you get
any notification?
>>> +
>>> + /* check if we should generate an error frame at all */
>>> + if (state == mod->can.state || mod->can.state == CAN_STATE_STOPPED) {
>>> + dev_dbg(mod->dev, "no error frame needed: state %d\n", state);
>>> + return;
>>> + }
>> Shouldn't this check be done first?
>>
>
> The variable "state" isn't set then. This code is basically copied from
> esd_pci331.c, but I moved the checking of priv->can.state out of each
> 'case', into one place.
>
>>> + dev_dbg(mod->dev, "state change: state %d\n", state);
>>> + mod->can.state = state;
>>> +
>>> + if (state == CAN_STATE_BUS_OFF) {
>>> + janz_err_frame(mod, CAN_ERR_BUSOFF, 0);
>>> + return;
>>> + }
>>> +
>>> + if (state == CAN_STATE_ERROR_PASSIVE) {
>>> + err = (rxerr >= 128) ? CAN_ERR_CRTL_RX_PASSIVE
>>> + : CAN_ERR_CRTL_TX_PASSIVE;
>>> + janz_err_frame(mod, CAN_ERR_CRTL, err);
>>> + return;
>>> + }
>>> +
>>> + if (state == CAN_STATE_ERROR_WARNING) {
>>> + err = (rxerr >= 96) ? CAN_ERR_CRTL_RX_WARNING
>>> + : CAN_ERR_CRTL_TX_WARNING;
>>> + janz_err_frame(mod, CAN_ERR_CRTL, err);
>>> + return;
>>> + }
>> If you use "else if" as suggested, the code could be shortened a lot, I
>> think (by doing everything within the if/else block).
>>
>> Just for curiosity, what does "candump -t d any,0:0,#FFFFFFFF" report
>> when you trigger a bus-error or when you send a message with no cable
>> connected.
>>
>
> Nothing. The device just eventually runs out of send buffers when no
> cable is connected. It doesn't send any error messages.
Hm, if you do not get any notification this code is useless.
>>> + /* nothing needed for error-active state */
>>> +}
>>> +
>>> +static void janz_handle_unknown(struct janz_ican3 *mod, struct janz_msg
>>> *msg)
>> Handle what?
>>
>
> The firmware has about 50 different messages it can send to the driver.
> I only handled the few that we expect to see. I put this in place so we
> get a log of message types that the driver doesn't handle yet.
>
> Should I remove it, and just drop all messages we don't handle without
> printing anything? Seems like a hindrance to debugging problems.
I have just a problem with the name, which is not specific enough. A
better name would be "janz_handle_unknown_message".
[snip]
>>> + switch (msg->spec) {
>>> + case MSG_IDVERS:
>>> + janz_handle_idvers(mod, msg);
>>> + break;
>>> + case MSG_MSGLOST:
>>> + case MSG_FMSGLOST:
>>> + janz_handle_msglost(mod, msg);
>>> + break;
>> You shoud create error messages for msglost as well.
>>
>
> Ok. What kind of messages? This message means that the driver is not
> reading CAN frames fast enough, and the firmware ran out of locations to
> store them. It is exactly the same as this driver's xmit() routine
> running out of buffers to place CAN messages for sending.
CAN_ERR_CRTL_RX_OVERFLOW and CAN_ERR_CRTL_RX_OVERFLOW from
http://lxr.linux.no/#linux+v2.6.32/include/linux/can/error.h.
[snip]
>>> + /* write back the control bits with IVALID unset */
>>> + control &= ~DESC_IVALID;
>>> + iowrite8(control, addr);
>> This seems to be duplicated code. Here a helper function would make
>> sense in contrast to your *one* line functions, e.g. to enable the
>> interrupts, which just increases code size.
>>
>
> The one-liners are marked inline, so they should get compiled to
> io(read|write)8(), without any function call overhead. I used them
> because their *names* are descriptive.
You do that in a few cases but in many other you use ioread8() directly.
I'm not worried about overhead.
> It is much easier to tell that this clears interrupts:
> janz_clr_int(mod);
>
> Than this:
> ioread8(mod->regs + DPM_REG_INT);
>
> Don't you think?
/* Clear interrupt */
ioread8(mod->regs + DPM_REG_INT);
does make pretty clear what the call does and I do not need to lookup
how it's implemented. If the interrupt is cleared in more than one place
or if it's a complex action, a function is useful, of course.
> I'm not sure how to clearly function-ize the clearing of the IVALID bit
> in descriptors, but I'll try for the next version.
Yes, a set of functions would be handy, I think.
[snip]
>>> + /* bring the bus online */
>>> + ret = janz_set_bus_state(mod, true);
>>> + if (ret) {
>>> + dev_err(mod->dev, "unable to set bus-on\n");
>>> + return ret;
>>> + }
>> How is bus-off recovery supposed to work? In general, if the card/hw
>> recovery automatically, we use the following procedure:
>>
>> restart_ms == 0: the device should be *stopped* on bus-off allowing
>> to user/app to restart it manually using this function.
>>
>> restart_ms > 0: the device is allowed to recover from bus-off
>> automatically.
>>
>> Could that be implemented?
>>
>
> The datasheet doesn't mention the word "restart" or "recover". I guess
> it doesn't recover automatically. Again, this code was copied from the
> esd_pci331.c driver. Is that driver's error recovery broken? What should
> I be doing here?
Makes sense as the SJA1000 does not recover automatically. But then some
kind of notification should be send otherwise the app does not known
what's going on.
>>> + /* disable our IRQ, then hookup the IRQ handler */
>>> + janz_disable_interrupts(mod);
>>> + ret = request_irq(mod->irq, janz_irq, IRQF_SHARED, DRV_NAME, mod);
>>> + if (ret) {
>>> + dev_err(dev, "unable to request IRQ\n");
>>> + goto out_iounmap_ctrl;
>>> + }
>> Is this interrupt exclisively for CAN? ... or do you need a dispatcher
>> in the MODULbus driver?
>>
>
> The interrupt is a function of the PCI bridge chip on the CMOD-IO
> carrier board. However, it uses a "write-1 to enable" and "write-1 to
> disable" interface, meaning each MODULbus module can have their own
> interrupt handler. The interrupt handler must check that its module is
> interrupting, just like any handler on a shared IRQ line.
>
> I don't need a dispatcher, but each module does need to know it's module
> number. See the janz_irq() function, and notice how I use a bitwise and
> with (1 << mod->num) to determine if this module is interrupting.
>
> AFAIK, Linux always calls all interrupt handlers attached to a shared
> interrupt line.
OK, that's fine.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
