* Re: [Nios2-dev] [PATCH] Altera TSE: Add missing phydev
From: Tobias Klauser @ 2014-12-25 14:55 UTC (permalink / raw)
To: Kostya Belezko; +Cc: Nios2-dev, Vince Bridgers, netdev
In-Reply-To: <20141225125239.GP16916@distanz.ch>
On 2014-12-25 at 13:52:39 +0100, Tobias Klauser <tklauser@distanz.ch> wrote:
> On 2014-12-22 at 23:37:19 +0100, Kostya Belezko <bkostya@hotmail.com> wrote:
> > Altera network device doesn't come up after
> >
> > ifconfig eth0 down
> > ifconfig eth0 up
> >
> > The reason behind is clearing priv->phydev during tse_shutdown().
> > The phydev is not restored back at tse_open().
> >
> > Signed-off-by: Kostya Belezko <bkostya@hotmail.com>
>
> Please Cc: netdev@vger.kernel.org for network driver patches.
>
> > ---
> > drivers/net/ethernet/altera/altera_tse_main.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> > index 4efc435..361bf35 100644
> > --- a/drivers/net/ethernet/altera/altera_tse_main.c
> > +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> > @@ -1150,6 +1150,9 @@ static int tse_open(struct net_device *dev)
> >
> > spin_unlock_irqrestore(&priv->rxdma_irq_lock, flags);
> >
> > + if (!priv->phydev)
> > + init_phy(dev);
>
> That way the PHY will be initialized twice on first start.
It actually won't. Sorry, I didn't read careful enough.
Still, the solution suggested below is preferable IMO and this is also
what other ethernet drivers are doing (at least those I checked).
> IMO the proper solution would be to disconnect and NULL the phydev in
> altera_tse_remove, not in tse_shutdown. There, only phy_stop should be
> called.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* NULL pointer dereference at skb_queue_tail()
From: Tetsuo Handa @ 2014-12-25 13:22 UTC (permalink / raw)
To: netdev
Hello.
I can reproduce below oops when testing Linux 3.18 with memory allocation
failure injection module at https://lkml.org/lkml/2014/12/25/64 .
Looks similar to http://oops.kernel.org/oops/bug-unable-to-handle-kernel-null-pointer-dereference-at-skb_queue_tail/ .
Where should I check?
----------
[ 273.709905] BUG: unable to handle kernel NULL pointer dereference at (null)
[ 273.713845] IP: [<ffffffff81535e27>] skb_queue_tail+0x37/0x60
[ 273.716720] PGD 7887d067 PUD 7bc5b067 PMD 0
[ 273.718647] Oops: 0002 [#1] SMP
[ 273.719508] Modules linked in: fault_injection(OE) ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 nf_conntrack_ipv6 nf_defrag_ipv6 ipt_REJECT nf_reject_ipv4 nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ebtable_nat ebtable_broute bridge stp llc ebtable_filter ebtables ip6table_mangle ip6table_raw ip6table_filter ip6_tables iptable_mangle iptable_raw iptable_filter ip_tables coretemp crct10dif_pclmul crc32_pclmul dm_mirror crc32c_intel dm_region_hash ghash_clmulni_intel dm_log aesni_intel glue_helper dm_mod lrw gf128mul ablk_helper cryptd vmw_balloon ppdev microcode parport_pc serio_raw pcspkr vmw_vmci parport i2c_piix4 shpchp nfsd auth_rpcgss nfs_acl lockd grace sunrpc uinput sd_mod ata_generic pata_acpi vmwgfx drm_kms_helper ttm drm mptspi e1000 scsi_transport_spi mptscsih mptba
se ata_piix libata i2c_core floppy [last unloaded: fault_injection]
[ 273.739290] CPU: 2 PID: 2866 Comm: Xorg Tainted: G W OE 3.18.0+ #337
[ 273.741001] Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/31/2013
[ 273.743534] task: ffff880079f18000 ti: ffff88007a894000 task.ti: ffff88007a894000
[ 273.745288] RIP: 0010:[<ffffffff81535e27>] [<ffffffff81535e27>] skb_queue_tail+0x37/0x60
[ 273.747275] RSP: 0018:ffff88007a897c18 EFLAGS: 00010046
[ 273.748535] RAX: 0000000000000296 RBX: ffff8800360c0b10 RCX: 0000000000000000
[ 273.750216] RDX: 0000000000000000 RSI: 0000000000000296 RDI: ffff8800360c0b24
[ 273.751921] RBP: ffff88007a897c38 R08: 0000000000000296 R09: 0000000000000300
[ 273.753624] R10: ffff88007f803600 R11: ffff88007a9dbd00 R12: ffff8800360c0b10
[ 273.755336] R13: ffff8800360c0b24 R14: 0000000000000000 R15: 0000000000000000
[ 273.757046] FS: 00007f512a6b0980(0000) GS:ffff88007fc80000(0000) knlGS:0000000000000000
[ 273.758940] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 273.760295] CR2: 0000000000000000 CR3: 000000007bf4f000 CR4: 00000000000407e0
[ 273.762047] Stack:
[ 273.762541] 0000000000000020 ffff8800360c0b10 0000000000000020 ffff8800360c0a80
[ 273.764392] ffff88007a897cf8 ffffffff815e9b11 ffff880048861498 ffff8800360c0b10
[ 273.766256] 0000002000000296 ffff88007a897d08 0000000000000020 ffff8800360c0d78
[ 273.768099] Call Trace:
[ 273.768695] [<ffffffff815e9b11>] unix_stream_sendmsg+0x1d1/0x420
[ 273.770157] [<ffffffff8152cf2a>] sock_aio_write+0xca/0xe0
[ 273.771472] [<ffffffff811af8bc>] do_sync_readv_writev+0x4c/0x80
[ 273.772910] [<ffffffff811b1255>] do_readv_writev+0x1e5/0x280
[ 273.774277] [<ffffffffa01656d5>] ? vmw_unlocked_ioctl+0x15/0x20 [vmwgfx]
[ 273.775899] [<ffffffff811c2f40>] ? do_vfs_ioctl+0x2e0/0x4c0
[ 273.777254] [<ffffffff811ccfa5>] ? __fget_light+0x25/0x70
[ 273.778557] [<ffffffff81100e84>] ? __audit_syscall_entry+0xb4/0x110
[ 273.780056] [<ffffffff811b1379>] vfs_writev+0x39/0x50
[ 273.781492] [<ffffffff811b14aa>] SyS_writev+0x4a/0xd0
[ 273.782741] [<ffffffff81647729>] system_call_fastpath+0x12/0x17
[ 273.784192] Code: 8d 6f 14 41 54 49 89 f4 53 48 89 fb 4c 89 ef 48 83 ec 08 e8 ec 13 11 00 48 8b 53 08 49 89 1c 24 4c 89 ef 48 89 c6 49 89 54 24 08 <4c> 89 22 83 43 10 01 4c 89 63 08 e8 19 10 11 00 48 83 c4 08 5b
[ 273.790477] RIP [<ffffffff81535e27>] skb_queue_tail+0x37/0x60
[ 273.791954] RSP <ffff88007a897c18>
[ 273.792798] CR2: 0000000000000000
----------
----------
crash> bt -l
PID: 2866 TASK: ffff880079f18000 CPU: 2 COMMAND: "Xorg"
#0 [ffff88007a8977f0] machine_kexec at ffffffff8104d092
/root/linux/arch/x86/kernel/machine_kexec_64.c: 319
#1 [ffff88007a897840] crash_kexec at ffffffff810ea6d3
/root/linux/kernel/kexec.c: 1482
#2 [ffff88007a897910] oops_end at ffffffff81016678
/root/linux/arch/x86/kernel/dumpstack.c: 231
#3 [ffff88007a897940] no_context at ffffffff8163bbc2
/root/linux/arch/x86/mm/fault.c: 724
#4 [ffff88007a8979a0] __bad_area_nosemaphore at ffffffff8163bc99
/root/linux/arch/x86/mm/fault.c: 804
#5 [ffff88007a8979f0] bad_area at ffffffff8163be4b
/root/linux/arch/x86/mm/fault.c: 833
#6 [ffff88007a897a20] __do_page_fault at ffffffff81057933
/root/linux/arch/x86/mm/fault.c: 1220
#7 [ffff88007a897b30] do_page_fault at ffffffff810579f1
/root/linux/arch/x86/mm/fault.c: 1299
#8 [ffff88007a897b60] page_fault at ffffffff816495e8
/root/linux/arch/x86/kernel/entry_64.S: 1255
[exception RIP: skb_queue_tail+55]
RIP: ffffffff81535e27 RSP: ffff88007a897c18 RFLAGS: 00010046
RAX: 0000000000000296 RBX: ffff8800360c0b10 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000296 RDI: ffff8800360c0b24
RBP: ffff88007a897c38 R8: 0000000000000296 R9: 0000000000000300
R10: ffff88007f803600 R11: ffff88007a9dbd00 R12: ffff8800360c0b10
R13: ffff8800360c0b24 R14: 0000000000000000 R15: 0000000000000000
ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018
#9 [ffff88007a897c40] unix_stream_sendmsg at ffffffff815e9b11
/root/linux/net/unix/af_unix.c: 1712
#10 [ffff88007a897d00] sock_aio_write at ffffffff8152cf2a
/root/linux/net/socket.c: 980
#11 [ffff88007a897d90] do_sync_readv_writev at ffffffff811af8bc
/root/linux/fs/read_write.c: 685
#12 [ffff88007a897e20] do_readv_writev at ffffffff811b1255
/root/linux/fs/read_write.c: 839
#13 [ffff88007a897f20] vfs_writev at ffffffff811b1379
/root/linux/fs/read_write.c: 881
#14 [ffff88007a897f30] sys_writev at ffffffff811b14aa
/root/linux/fs/read_write.c: 914
#15 [ffff88007a897f80] system_call_fastpath at ffffffff81647729
/root/linux/arch/x86/kernel/entry_64.S: 423
RIP: 00007f51285923c0 RSP: 00007fff324a0e00 RFLAGS: 00013202
RAX: ffffffffffffffda RBX: ffffffff81647729 RCX: 00000000004c66f0
RDX: 0000000000000001 RSI: 00007fff324a0840 RDI: 0000000000000013
RBP: 000000000210e4b0 R8: 0000000000000000 R9: 0000000000400000
R10: 0000000000000000 R11: 0000000000003293 R12: 00007f512a6b06a0
R13: 0000000000000001 R14: 00007fff324a0840 R15: 0000000000000000
ORIG_RAX: 0000000000000014 CS: 0033 SS: 002b
----------
void skb_queue_tail(struct sk_buff_head *list, struct sk_buff *newsk)
{
unsigned long flags;
spin_lock_irqsave(&list->lock, flags);
__skb_queue_tail(list, newsk);
spin_unlock_irqrestore(&list->lock, flags);
}
static inline void __skb_queue_tail(struct sk_buff_head *list,
struct sk_buff *newsk)
{
__skb_queue_before(list, (struct sk_buff *)list, newsk);
}
static inline void __skb_queue_before(struct sk_buff_head *list,
struct sk_buff *next,
struct sk_buff *newsk)
{
__skb_insert(newsk, next->prev, next, list);
}
static inline void __skb_insert(struct sk_buff *newsk,
struct sk_buff *prev, struct sk_buff *next,
struct sk_buff_head *list)
{
newsk->next = next;
newsk->prev = prev;
next->prev = prev->next = newsk; // <= ffffffff81535e27 is here.
list->qlen++;
}
^ permalink raw reply
* Re: [Nios2-dev] [PATCH] Altera TSE: Add missing phydev
From: Tobias Klauser @ 2014-12-25 12:52 UTC (permalink / raw)
To: Kostya Belezko; +Cc: Nios2-dev, Vince Bridgers, netdev
In-Reply-To: <BLU436-SMTP58313D55680097A2C4AAA0B6560@phx.gbl>
On 2014-12-22 at 23:37:19 +0100, Kostya Belezko <bkostya@hotmail.com> wrote:
> Altera network device doesn't come up after
>
> ifconfig eth0 down
> ifconfig eth0 up
>
> The reason behind is clearing priv->phydev during tse_shutdown().
> The phydev is not restored back at tse_open().
>
> Signed-off-by: Kostya Belezko <bkostya@hotmail.com>
Please Cc: netdev@vger.kernel.org for network driver patches.
> ---
> drivers/net/ethernet/altera/altera_tse_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/net/ethernet/altera/altera_tse_main.c b/drivers/net/ethernet/altera/altera_tse_main.c
> index 4efc435..361bf35 100644
> --- a/drivers/net/ethernet/altera/altera_tse_main.c
> +++ b/drivers/net/ethernet/altera/altera_tse_main.c
> @@ -1150,6 +1150,9 @@ static int tse_open(struct net_device *dev)
>
> spin_unlock_irqrestore(&priv->rxdma_irq_lock, flags);
>
> + if (!priv->phydev)
> + init_phy(dev);
That way the PHY will be initialized twice on first start.
IMO the proper solution would be to disconnect and NULL the phydev in
altera_tse_remove, not in tse_shutdown. There, only phy_stop should be
called.
^ permalink raw reply
* [PATCH 1/1 net-next] tipc: replace 0 by NULL for pointers
From: Fabian Frederick @ 2014-12-25 11:05 UTC (permalink / raw)
To: linux-kernel
Cc: Fabian Frederick, Jon Maloy, Allan Stephens, David S. Miller,
netdev, tipc-discussion
Fix sparse warning:
net/tipc/link.c:1924:40: warning: Using plain integer as NULL pointer
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---
net/tipc/link.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 23bcc11..082c3b5 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1921,7 +1921,7 @@ static struct tipc_node *tipc_link_find_owner(const char *link_name,
{
struct tipc_link *l_ptr;
struct tipc_node *n_ptr;
- struct tipc_node *found_node = 0;
+ struct tipc_node *found_node = NULL;
int i;
*bearer_id = 0;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 00/10] Split UFO into v4 and v6 versions.
From: Jason Wang @ 2014-12-25 9:50 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, Ben Hutchings, stefanha,
virtualization
In-Reply-To: <20141225071435.GA6687@redhat.com>
On Thu, Dec 25, 2014 at 3:14 PM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Thu, Dec 25, 2014 at 03:10:01AM +0008, Jason Wang wrote:
>>
>>
>> On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin
>> <mst@redhat.com> wrote:
>> >On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
>> >> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
>> >> > > ----- Original Message -----
>> >> > > UFO support in the kernel applies to both IPv4 and IPv6
>> protocols
>> >> > > with the same device feature. However some devices may not
>> be able
>> >> > > to support one of the offloads. For this we split the UFO
>> offload
>> >> > > feature into 2 pieces. NETIF_F_UFO now controlls the IPv4
>> part and
>> >> > > this series introduces NETIF_F_UFO6.
>> >> > > > > As a result of this work, we can now re-enable
>> NETIF_F_UFO on
>> >> > > virtio_net devices and restore UDP over IPv4 performance for
>> >>guests.
>> >> > > We also continue to support legacy guests that assume that
>> UFO6
>> >> > > support included into UFO(4).
>> >> > > > > Without this work, migrating a guest to a 3.18 kernel
>> fails.
>> >> > > > > This series eliminate the ambiguous NETIF_F_UFO.
>> >> > > But VIRTIO_NET_F_{HOST|GUEST}_UFO and
>> VIRTIO_NET_F_HDR_GSO_UDP is
>> >>still
>> >> > ambigious. I know it was used to keep compatibility for legacy
>> guest.
>> >>But
>> >> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio
>> >>features and
>> >> > gso type in vnet header looks sufficient?
>> >> [...]
>> >> The IPv6 fragmentation ID needs to be added to the vnet
>> header, to do
>> >> UFOv6 properly. If it wasn't for that lack, we wouldn't have to
>> split
>> >> the feature flag.
>> >> Ben.
>> >
>> >
>> >Right.
>> >
>> >I think a good plan is as follows:
>> >
>> >1. add code generating IDs on rx path for virtio,
>> > and re-enable UFO (4+6).
>> >
>> >2. Ben's patch doing similar things for tun/macvtap
>>
>> I wonder whether or not we can do this lazily.
>>
>> E.g during UFO6 software segment for dodgy packets?
>>
>> And this can eliminate the effort of duplicating it in all untrusted
>> sources?
>
> But then we'll need to then change it again in step 5,
> probably by setting some flag?
The idea is if no segmentation (either software or hardware), no need
for an id.
So looks like dodgy is sufficient since we are sure kernel should set
id in all other cases. And we can add the id only when we see it was
not set for the ufo packet(legacy guest). This seems not conflict with
step 5.
>
> Might as well do it directly.
>
>> >
>> >3. similarly for packet sockets
>> >
>> >
>> >above seem appropriate for stable
>> >
>> >4. 4+6 split, to make it easier for drivers
>> > to identify when fragment ID is needed
>> >
>> >5. extend vnet header, and add ways to enable it,
>> > when enabled, use that to pass
>> > fragment ID for tun/virtio/vhost/macvtap/packet socket
>> >
>> >4 is what this patchset does.
>> >
>> >
>> >> -- Ben Hutchings
>> >> If more than one person is responsible for a bug, no one is at
>> fault.
>> >
>> >
>> >--
>> >To unsubscribe from this list: send the line "unsubscribe netdev"
>> in
>> >the body of a message to majordomo@vger.kernel.org
>> >More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/4] can: kvaser_usb: Don't free packets when tight on URBs
From: Ahmed S. Darwish @ 2014-12-25 9:38 UTC (permalink / raw)
To: Greg KH
Cc: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, Paul Gortmaker, Linux-CAN,
netdev, Linux-stable, LKML
In-Reply-To: <20141225025011.GA10491@kroah.com>
On Wed, Dec 24, 2014 at 06:50:11PM -0800, Greg KH wrote:
> On Thu, Dec 25, 2014 at 01:56:44AM +0200, Ahmed S. Darwish wrote:
> > From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> >
> > Flooding the Kvaser CAN to USB dongle with multiple reads and
> > writes in high frequency caused seemingly-random panics in the
> > kernel.
> >
> > On further inspection, it seems the driver erroneously freed the
> > to-be-transmitted packet upon getting tight on URBs and returning
> > NETDEV_TX_BUSY, leading to invalid memory writes and double frees
> > at a later point in time.
> >
> > Note:
> >
> > Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
> > is a driver bug in and out of itself: it means that our start/stop
> > queue flow control is broken.
> >
> > This patch only fixes the (buggy) error handling code; the root
> > cause shall be fixed in a later commit.
> >
> > Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> > ---
> > drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > (Marc, Greg, I believe this should also be added to -stable?)
>
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.
>
> </formletter>
<msg-response>
Note taken. Sorry about that ;-)
</msg-response>
^ permalink raw reply
* Re: [PATCH 00/10] Split UFO into v4 and v6 versions.
From: Michael S. Tsirkin @ 2014-12-25 7:14 UTC (permalink / raw)
To: Jason Wang
Cc: netdev, Vladislav Yasevich, Ben Hutchings, stefanha,
virtualization
In-Reply-To: <1419476521.27992.1@smtp.corp.redhat.com>
On Thu, Dec 25, 2014 at 03:10:01AM +0008, Jason Wang wrote:
>
>
> On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> >On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
> >> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
> >> > > ----- Original Message -----
> >> > > UFO support in the kernel applies to both IPv4 and IPv6 protocols
> >> > > with the same device feature. However some devices may not be able
> >> > > to support one of the offloads. For this we split the UFO offload
> >> > > feature into 2 pieces. NETIF_F_UFO now controlls the IPv4 part and
> >> > > this series introduces NETIF_F_UFO6.
> >> > > > > As a result of this work, we can now re-enable NETIF_F_UFO on
> >> > > virtio_net devices and restore UDP over IPv4 performance for
> >>guests.
> >> > > We also continue to support legacy guests that assume that UFO6
> >> > > support included into UFO(4).
> >> > > > > Without this work, migrating a guest to a 3.18 kernel fails.
> >> > > > > This series eliminate the ambiguous NETIF_F_UFO.
> >> > > But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is
> >>still
> >> > ambigious. I know it was used to keep compatibility for legacy guest.
> >>But
> >> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio
> >>features and
> >> > gso type in vnet header looks sufficient?
> >> [...]
> >> The IPv6 fragmentation ID needs to be added to the vnet header, to do
> >> UFOv6 properly. If it wasn't for that lack, we wouldn't have to split
> >> the feature flag.
> >> Ben.
> >
> >
> >Right.
> >
> >I think a good plan is as follows:
> >
> >1. add code generating IDs on rx path for virtio,
> > and re-enable UFO (4+6).
> >
> >2. Ben's patch doing similar things for tun/macvtap
>
> I wonder whether or not we can do this lazily.
>
> E.g during UFO6 software segment for dodgy packets?
>
> And this can eliminate the effort of duplicating it in all untrusted
> sources?
But then we'll need to then change it again in step 5,
probably by setting some flag?
Might as well do it directly.
> >
> >3. similarly for packet sockets
> >
> >
> >above seem appropriate for stable
> >
> >4. 4+6 split, to make it easier for drivers
> > to identify when fragment ID is needed
> >
> >5. extend vnet header, and add ways to enable it,
> > when enabled, use that to pass
> > fragment ID for tun/virtio/vhost/macvtap/packet socket
> >
> >4 is what this patchset does.
> >
> >
> >> -- Ben Hutchings
> >> If more than one person is responsible for a bug, no one is at fault.
> >
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe netdev" in
> >the body of a message to majordomo@vger.kernel.org
> >More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH net v2] net: Generalize ndo_gso_check tondo_features_check
From: Hayes Wang @ 2014-12-25 3:29 UTC (permalink / raw)
To: Jesse Gross, David Miller
Cc: netdev@vger.kernel.org, Tom Herbert, Joe Stringer, Eric Dumazet
In-Reply-To: <1419403046-43165-1-git-send-email-jesse@nicira.com>
Jesse Gross [mailto:jesse@nicira.com]
> Sent: Wednesday, December 24, 2014 2:37 PM
[...]
> GSO isn't the only offload feature with restrictions that
> potentially can't be expressed with the current features mechanism.
> Checksum is another although it's a general issue that could in
> theory apply to anything. Even if it may be possible to
> implement these restrictions in other ways, it can result in
> duplicate code or inefficient per-packet behavior.
>
> This generalizes ndo_gso_check so that drivers can remove any
> features that don't make sense for a given packet, similar to
> netif_skb_features(). It also converts existing driver
> restrictions to the new format, completing the work that was
> done to support tunnel protocols since the issues apply to
> checksums as well.
>
> By actually removing features from the set that are used to do
> offloading, it solves another problem with the existing
> interface. In these cases, GSO would run with the original set
> of features and not do anything because it appears that
> segmentation is not required.
>
> CC: Tom Herbert <therbert@google.com>
> CC: Joe Stringer <joestringer@nicira.com>
> CC: Eric Dumazet <edumazet@google.com>
> CC: Hayes Wang <hayeswang@realtek.com>
> Signed-off-by: Jesse Gross <jesse@nicira.com>
> Acked-by: Tom Herbert <therbert@google.com>
> Fixes: 04ffcb255f22 ("net: Add ndo_gso_check")
> ---
> v2: Rebase
> Drop overzealous use of netdev_intersect_features()
Tested-by: Hayes Wang <hayeswang@realtek.com>
^ permalink raw reply
* Re: [PATCH 00/10] Split UFO into v4 and v6 versions.
From: Jason Wang @ 2014-12-25 3:02 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, Vladislav Yasevich, Ben Hutchings, stefanha,
virtualization
In-Reply-To: <20141224185922.GA30934@redhat.com>
On Thu, Dec 25, 2014 at 2:59 AM, Michael S. Tsirkin <mst@redhat.com>
wrote:
> On Wed, Dec 24, 2014 at 07:11:20PM +0100, Ben Hutchings wrote:
>> On Thu, 2014-12-18 at 00:28 -0500, Jason Wang wrote:
>> >
>> > ----- Original Message -----
>> > > UFO support in the kernel applies to both IPv4 and IPv6
>> protocols
>> > > with the same device feature. However some devices may not be
>> able
>> > > to support one of the offloads. For this we split the UFO
>> offload
>> > > feature into 2 pieces. NETIF_F_UFO now controlls the IPv4 part
>> and
>> > > this series introduces NETIF_F_UFO6.
>> > >
>> > > As a result of this work, we can now re-enable NETIF_F_UFO on
>> > > virtio_net devices and restore UDP over IPv4 performance for
>> guests.
>> > > We also continue to support legacy guests that assume that UFO6
>> > > support included into UFO(4).
>> > >
>> > > Without this work, migrating a guest to a 3.18 kernel fails.
>> > >
>> >
>> > This series eliminate the ambiguous NETIF_F_UFO.
>> >
>> > But VIRTIO_NET_F_{HOST|GUEST}_UFO and VIRTIO_NET_F_HDR_GSO_UDP is
>> still
>> > ambigious. I know it was used to keep compatibility for legacy
>> guest. But
>> > what's the future plan? Differentiate UFOv4 and UFOv6 in virtio
>> features and
>> > gso type in vnet header looks sufficient?
>> [...]
>>
>> The IPv6 fragmentation ID needs to be added to the vnet header, to
>> do
>> UFOv6 properly. If it wasn't for that lack, we wouldn't have to
>> split
>> the feature flag.
>>
>> Ben.
>
>
> Right.
>
> I think a good plan is as follows:
>
> 1. add code generating IDs on rx path for virtio,
> and re-enable UFO (4+6).
>
> 2. Ben's patch doing similar things for tun/macvtap
I wonder whether or not we can do this lazily.
E.g during UFO6 software segment for dodgy packets?
And this can eliminate the effort of duplicating it in all untrusted
sources?
>
> 3. similarly for packet sockets
>
>
> above seem appropriate for stable
>
> 4. 4+6 split, to make it easier for drivers
> to identify when fragment ID is needed
>
> 5. extend vnet header, and add ways to enable it,
> when enabled, use that to pass
> fragment ID for tun/virtio/vhost/macvtap/packet socket
>
> 4 is what this patchset does.
>
>
>> --
>> Ben Hutchings
>> If more than one person is responsible for a bug, no one is at
>> fault.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 1/4] can: kvaser_usb: Don't free packets when tight on URBs
From: Greg KH @ 2014-12-25 2:50 UTC (permalink / raw)
To: Ahmed S. Darwish
Cc: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde, David S. Miller, Paul Gortmaker, Linux-CAN,
netdev, Linux-stable, LKML
In-Reply-To: <20141224235644.GA3778@vivalin-002>
On Thu, Dec 25, 2014 at 01:56:44AM +0200, Ahmed S. Darwish wrote:
> From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
>
> Flooding the Kvaser CAN to USB dongle with multiple reads and
> writes in high frequency caused seemingly-random panics in the
> kernel.
>
> On further inspection, it seems the driver erroneously freed the
> to-be-transmitted packet upon getting tight on URBs and returning
> NETDEV_TX_BUSY, leading to invalid memory writes and double frees
> at a later point in time.
>
> Note:
>
> Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
> is a driver bug in and out of itself: it means that our start/stop
> queue flow control is broken.
>
> This patch only fixes the (buggy) error handling code; the root
> cause shall be fixed in a later commit.
>
> Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
> ---
> drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> (Marc, Greg, I believe this should also be added to -stable?)
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read Documentation/stable_kernel_rules.txt
for how to do this properly.
</formletter>
^ permalink raw reply
* Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC
From: Chen-Yu Tsai @ 2014-12-25 2:32 UTC (permalink / raw)
To: Roger
Cc: Heiko Stübner, Giuseppe Cavallaro, netdev, linux-kernel,
linux-rockchip, kever.yang, eddie.cai
In-Reply-To: <547EC24E.6050905@rock-chips.com>
Hi,
On Wed, Dec 3, 2014 at 3:57 PM, Roger <roger.chen@rock-chips.com> wrote:
> Hi! Heiko
>
> about regulator, power gpio, reset gpio and irq gpio
> please refer to my comment inline, tks.
>
>
> On 2014/12/2 7:44, Heiko Stübner wrote:
>>
>> Hi Roger,
>>
>> the comments inline are a rough first review. I hope to get a clearer
>> picture
>> for the stuff I'm not sure about in v3 once the big issues are fixed.
>>
>>
>> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>>>
>>> This driver is based on stmmac driver.
>>>
>>> modification based on Giuseppe CAVALLARO's suggestion:
>>> 1. use BIT()
>>>
>>> > +/*RK3288_GRF_SOC_CON3*/
>>> > +#define GMAC_TXCLK_DLY_ENABLE ((0x4000 << 16) | (0x4000))
>>> > +#define GMAC_TXCLK_DLY_DISABLE ((0x4000 << 16) | (0x0000))
>>>
>>> ...
>>>
>>> why do not use BIT and BIT_MASK where possible?
>>>
>>> ===>after modification:
>>>
>>> #define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> #define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> #define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> #define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> ...
>>> 2.
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_PHY_INTF_SEL_RGMII);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_RMII_MODE_CLR);
>>>
>>> maybe you could perform just one write unless there is some HW
>>> constraint.
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>>
>>> 3. use macros
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E,
>>> 0xFFFFFFFF);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>> > + 0x3<<2<<16 | 0x3<<2);
>>>
>>> pls use macros, these shift sequence is really help to decode
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>>
>>> 4. remove grf fail check in rk_gmac_setup()
>>>
>>> > + if (IS_ERR(bsp_priv->grf))
>>> > + dev_err(&pdev->dev, "Missing rockchip,grf
>>> property\n");
>>>
>>> I wonder if you can fail on here and save all the check in
>>> set_rgmii_speed etc.
>>> Maybe this can be considered a mandatory property for the
>>> glue-logic.
>>>
>>> 5. remove .tx_coe=1
>>>
>>> > +const struct stmmac_of_data rk_gmac_data = {
>>> > + .has_gmac = 1,
>>> > + .tx_coe = 1,
>>>
>>> FYI, on new gmac there is the HW capability register to
>>> dinamically
>>> provide you if coe is supported.
>>>
>>> IMO you should add the OF "compatible" string and in case of mac
>>> newer than the 3.50a you can remove coe.
>>
>> changelogs like these, should be compact and also not be in the commit
>> message
>> itself, but in the "comment"-section below the "---" and before the
>> diffstat.
>>
>>
>>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>>> ---
>>
>> changelog here ... the commonly used pattern is something like
>>
>> changes since v2:
>> - ...
>> - ...
>>
>> changes since v1:
>> - ...
>>
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
>>> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 636
>>> ++++++++++++++++++++ .../net/ethernet/stmicro/stmmac/stmmac_platform.c |
>>> 3 +
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.h | 1 +
>>> 4 files changed, 641 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>>> 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
>>> stmmac_mdio.o
>>> ring_mode.o \
>>>
>>> obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>> stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o dwmac-sunxi.o \
>>> - dwmac-sti.o dwmac-socfpga.o
>>> + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>> obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>> stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>>> index 0000000..870563f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,636 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen) <roger.chen@rock-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct rk_priv_data {
>>> + struct platform_device *pdev;
>>> + int phy_iface;
>>> + bool power_ctrl_by_pmu;
>>> + char pmu_regulator[32];
>>> + int pmu_enable_level;
>>> +
>>> + int power_io;
>>> + int power_io_level;
>>> + int reset_io;
>>> + int reset_io_level;
>>> + int phyirq_io;
>>> + int phyirq_io_level;
>>> +
>>> + bool clk_enabled;
>>> + bool clock_input;
>>> +
>>> + struct clk *clk_mac;
>>> + struct clk *clk_mac_pll;
>>> + struct clk *gmac_clkin;
>>> + struct clk *mac_clk_rx;
>>> + struct clk *mac_clk_tx;
>>> + struct clk *clk_mac_ref;
>>> + struct clk *clk_mac_refout;
>>> + struct clk *aclk_mac;
>>> + struct clk *pclk_mac;
>>> +
>>> + int tx_delay;
>>> + int rx_delay;
>>> +
>>> + struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>>
>> here you're using a space instead of a tab, please select one pattern
>> either
>> tabs or space but do not mix them.
>>
>>
>>> +#define GPIO3D_2MA 0xFFFF0000
>>> +#define GPIO3D_4MA 0xFFFF5555
>>> +#define GPIO3D_8MA 0xFFFFAAAA
>>> +#define GPIO3D_12MA 0xFFFFFFFF
>>> +
>>> +#define GPIO4A_2MA 0xFFFF0000
>>> +#define GPIO4A_4MA 0xFFFF5555
>>> +#define GPIO4A_8MA 0xFFFFAAAA
>>> +#define GPIO4A_12MA 0xFFFFFFFF
>>
>> see comment about pin settings below
>>
>>
>>> +
>>> +#define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> +#define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> +
>>> +#define GPIO4B_2_2MA (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_4MA (GRF_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_8MA (GRF_CLR_BIT(2) | GRF_BIT(3))
>>> +#define GPIO4B_2_12MA (GRF_BIT(2) | GRF_BIT(3))
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII (GRF_BIT(6) | GRF_CLR_BIT(7) |
>>> GRF_CLR_BIT(8))
>>> +#define GMAC_PHY_INTF_SEL_RMII (GRF_CLR_BIT(6) |
>>> GRF_CLR_BIT(7) | GRF_BIT(8))
>>> +#define GMAC_FLOW_CTRL GRF_BIT(9)
>>> +#define GMAC_FLOW_CTRL_CLR GRF_CLR_BIT(9)
>>> +#define GMAC_SPEED_10M GRF_CLR_BIT(10)
>>> +#define GMAC_SPEED_100M GRF_BIT(10)
>>> +#define GMAC_RMII_CLK_25M GRF_BIT(11)
>>> +#define GMAC_RMII_CLK_2_5M GRF_CLR_BIT(11)
>>> +#define GMAC_CLK_125M (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>>> +#define GMAC_CLK_25M (GRF_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_CLK_2_5M (GRF_CLR_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_RMII_MODE GRF_BIT(14)
>>> +#define GMAC_RMII_MODE_CLR GRF_CLR_BIT(14)
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> +#define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> +#define GMAC_RXCLK_DLY_ENABLE GRF_BIT(15)
>>> +#define GMAC_RXCLK_DLY_DISABLE GRF_CLR_BIT(15)
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
>>
>> again mixed tabs and spaces as delimiters.
>>
>> Also the _CFG macros are not well abstracted. You could take a look at the
>> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>>
>> #define GMAC_CLK_DL_MASK 0x7f
>> #define GMAC_CLK_RX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
>> #define GMAC_CLK_TX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>>
>>
>>> +
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> + int tx_delay, int rx_delay)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>>> + GMAC_CLK_RX_DL_CFG(rx_delay) |
>>> + GMAC_CLK_TX_DL_CFG(tx_delay));
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>
>> please don't write to parts controlled by other drivers - here the drive
>> strength settings of pins is controlled by the pinctrl driver. Instead you
>> can
>> just set the drive-strength in the pinctrl settings.
>>
>>
>>> +
>>> + pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> + __func__, tx_delay, rx_delay);
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>
>> you have a device-reference in rk_priv_data, so you could use dev_err
>> here.
>> Same for all other pr_err/pr_debug/pr_* calls in this file.
>>
>>
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RMII);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_MODE);
>>
>> these two could be combined?
>>
>>
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_2_5M);
>>> + else if (speed == 100)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_25M);
>>> + else if (speed == 1000)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_125M);
>>> + else
>>> + pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_2_5M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_10M);
>>
>> combine into one write?
>>
>>
>>> + } else if (speed == 100) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_25M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_100M);
>>
>> combine into one write?
>>
>>
>>> + } else {
>>> + pr_err("unknown speed value for RMII! speed=%d", speed);
>>> + }
>>> +}
>>> +
>>> +#define MAC_CLK_RX "mac_clk_rx"
>>> +#define MAC_CLK_TX "mac_clk_tx"
>>> +#define CLK_MAC_REF "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT "clk_mac_refout"
>>> +#define CLK_MAC_PLL "clk_mac_pll"
>>> +#define ACLK_MAC "aclk_mac"
>>> +#define PCLK_MAC "pclk_mac"
>>> +#define MAC_CLKIN "ext_gmac"
>>> +#define CLK_MAC "stmmaceth"
>>
>> why the need to extra constants for the clock names and not use the real
>> names
>> directly like most other drivers do?
>>
>>
>>> +
>>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> + struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> + bsp_priv->clk_enabled = false;
>>> +
>>> + bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> + if (IS_ERR(bsp_priv->mac_clk_rx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_RX);
>>> +
>>> + bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> + if (IS_ERR(bsp_priv->mac_clk_tx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_TX);
>>> +
>>> + bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> + if (IS_ERR(bsp_priv->clk_mac_ref))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF);
>>> +
>>> + bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> + if (IS_ERR(bsp_priv->clk_mac_refout))
>>> + pr_warn("%s: warning:cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF_OUT);
>>> +
>>> + bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> + if (IS_ERR(bsp_priv->aclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, ACLK_MAC);
>>> +
>>> + bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> + if (IS_ERR(bsp_priv->pclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, PCLK_MAC);
>>> +
>>> + bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> + if (IS_ERR(bsp_priv->clk_mac_pll))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_PLL);
>>> +
>>> + bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> + if (IS_ERR(bsp_priv->gmac_clkin))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLKIN);
>>> +
>>> + bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> + if (IS_ERR(bsp_priv->clk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC);
>>
>> there is not clk_put in the _remove case ... maybe you could simply use
>> devm_clk_get here so that all clocks are put on device removal.
>>
>> Also you're warning on every missing clock. Below it looks like you need a
>> different set of them for rgmii and rmii, so maybe you should simply error
>> out
>> when core clocks for the selected phy-mode are missing.
>>
>>
>>> +
>>> + if (bsp_priv->clock_input) {
>>> + pr_info("%s: clock input from PHY\n", __func__);
>>> + } else {
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> + clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>>
>> why the explicit reparenting. The common clock-framework is intelligent
>> enough
>> to select the best suitable parent.
>>
>> In general I'm thinking the clock-handling inside this driver should be
>> simplyfied, as the common-clock framework can handle most cases itself.
>> I.e. if
>> a 125MHz external clock is present and so on. But haven't looked to deep
>> yet.
>>
>>
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> + if (enable) {
>>> + if (!bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_prepare_enable(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_prepare_enable(
>>> +
>>> bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_prepare_enable(bsp_priv->clk_mac);
>>> + */
>>> + mdelay(5);
>>> + bsp_priv->clk_enabled = true;
>>> + }
>>> + } else {
>>> + if (bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_disable_unprepare(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_disable_unprepare(
>>> +
>>> bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> +
>>> clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> +
>>> clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> +
>>> clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_disable_unprepare(bsp_priv->clk_mac);
>>> + */
>>> + bsp_priv->clk_enabled = false;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + struct regulator *ldo;
>>> + char *ldostr = bsp_priv->pmu_regulator;
>>> + int ret;
>>> +
>>> + if (!ldostr) {
>>> + pr_err("%s: no ldo found\n", __func__);
>>> + return -1;
>>> + }
>>> +
>>> + ldo = regulator_get(NULL, ldostr);
>>> + if (!ldo) {
>>> + pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> + } else {
>>> + if (enable) {
>>> + if (!regulator_is_enabled(ldo)) {
>>> + regulator_set_voltage(ldo, 3300000,
>>> 3300000);
>>> + ret = regulator_enable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to enable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn on ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is enabled before enable",
>>> ldostr);
>>> + }
>>> + } else {
>>> + if (regulator_is_enabled(ldo)) {
>>> + ret = regulator_disable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to disable
>>> %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn off ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is disabled before disable",
>>> + ldostr);
>>> + }
>>> + }
>>> + regulator_put(ldo);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + if (enable) {
>>> + /*power on*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + bsp_priv->power_io_level);
>>> + } else {
>>> + /*power off*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + !bsp_priv->power_io_level);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int ret = -1;
>>> +
>>> + pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> + if (bsp_priv->power_ctrl_by_pmu)
>>> + ret = power_on_by_pmu(bsp_priv, enable);
>>> + else
>>> + ret = power_on_by_gpio(bsp_priv, enable);
>>
>> this looks wrong. This should always be a regulator. Even a regulator +
>> switch
>> controlled by a gpio can still be modelled as regulator, so that you don't
>> need this switch and assorted special handling - so just use the regulator
>> API
>>
> In some case, it would be a switching circuit to control the power for PHY.
> All I need to do is to control a GPIO to make switch on/off. So...
A regulator would probably be a better choice. You can use fixed-regulator.
The regulator framework should also take care of enabling any upstream
regulators, such as an LDO output from the PMIC that powers some external
pull-ups or what not.
The sunxi glue driver has this support. Maybe you could move that to the
stmmac platform core.
>>>
>>> +
>>> + if (enable) {
>>> + /*reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + mdelay(5);
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + !bsp_priv->reset_io_level);
>>> + }
>>> + mdelay(30);
>>> +
>>> + } else {
>>> + /*pull down reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + }
>>> + }
>>
>> I'm not sure yet if it would be better to use the reset framework for
>> this.
>> While it says it is also meant for reset-gpios, there does not seem a
>> driver
>> for this to exist yet.
>>
> What should I do?
The stmmac driver has support for handling phy resets using gpios.
Please use it. See Documentation/devicetree/bindings/net/stmmac.txt
for details. I think this was discussed some time ago, though
probably in a different context. If it's just a gpio that needs to
be poked, it should stay a gpio, at least for external resets.
>
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER "gmac_phy_power"
>>> +#define GPIO_PHY_RESET "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ "gmac_phy_irq"
>>
>> again I don't understand why these constants are necessary
>>
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> + struct rk_priv_data *bsp_priv;
>>> + struct device *dev = &pdev->dev;
>>> + enum of_gpio_flags flags;
>>> + int ret;
>>> + const char *strings = NULL;
>>> + int value;
>>> + int irq;
>>> +
>>> + bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> + if (!bsp_priv)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> + ret = of_property_read_string(dev->of_node, "pmu_regulator",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_regulator.\n",
>>> __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: ethernet phy power controlled by
>>> pmu(%s).\n",
>>> + __func__, strings);
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + strcpy(bsp_priv->pmu_regulator, strings);
>>> + }
>>
>> There is a generic regulator-dt-binding for regulator-consumers available
>> which you should of course use.
>>
> The same explanation as above
>
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
>>> &value);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> + __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> + __func__, (value == 1) ? "HIGH" : "LOW");
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + bsp_priv->pmu_enable_level = value;
>>> + }
>>
>> What is this used for? Enabling should of course be done via
>> regulator_enable
>> and disabling using regulator_disable.
Second. Even if it's handled by the PMU, you should also model the
PMU as a set of regulators. That will get rid of all the ifs in
this section.
>>
>>
>>> +
>>> + ret = of_property_read_string(dev->of_node, "clock_in_out",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: clock_in_out.\n",
>>> __func__);
>>> + bsp_priv->clock_input = true;
>>> + } else {
>>> + pr_info("%s: clock input or output? (%s).\n",
>>> + __func__, strings);
>>> + if (!strcmp(strings, "input"))
>>> + bsp_priv->clock_input = true;
>>> + else
>>> + bsp_priv->clock_input = false;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->tx_delay = 0x30;
>>> + pr_err("%s: Can not read property: tx_delay.", __func__);
>>> + pr_err("%s: set tx_delay to 0x%x\n",
>>> + __func__, bsp_priv->tx_delay);
>>> + } else {
>>> + pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->tx_delay = value;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->rx_delay = 0x10;
>>> + pr_err("%s: Can not read property: rx_delay.", __func__);
>>> + pr_err("%s: set rx_delay to 0x%x\n",
>>> + __func__, bsp_priv->rx_delay);
>>> + } else {
>>> + pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->rx_delay = value;
>>> + }
>>> +
>>> + bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> + "rockchip,grf");
>>> + bsp_priv->phyirq_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "phyirq-gpio", 0, &flags);
>>> + bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->reset_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "reset-gpio", 0, &flags);
>>> + bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->power_io =
>>> + of_get_named_gpio_flags(dev->of_node, "power-gpio", 0,
>>> &flags);
>>> + bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + /*power*/
>>> + if (!gpio_is_valid(bsp_priv->power_io)) {
>>> + pr_err("%s: Failed to get GPIO %s.\n",
>>> + __func__, "power-gpio");
>>> + } else {
>>> + ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_POWER);
>>> + }
>>
>> When everything power-related is handled using the regulator api, you
>> don't
>> need this
>
> The same explanation as above
>>
>>
>>> +
>>> + if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> + pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> + } else {
>>> + ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_RESET);
>>> + }
>>> +
>>> + if (bsp_priv->phyirq_io > 0) {
>>
>> This is more for my understanding: why does the mac driver need to handle
>> the
>> phy interrupt - but I might be overlooking something.
stmmac does not have a separate mdio bus driver. They are combined.
>>
> phy interrupt is not mandatory. In most of the time, in order to find
> something happen in PHY, for example,
> link is up or down, we just use polling method to read the phy's register
> in a timer.
> Buf if phy interrupt is in use, when link is up or down, phy interrupt pin
> will be assert to inform the CPU.
> I just implement the driver for phy interrupt gpio, not enable it as
> default.
You should probably do it in the stmmac platform/core driver.
>
>
>>> + struct plat_stmmacenet_data *plat_dat;
>>> +
>>> + pr_info("PHY irq in use\n");
>>> + ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Failed to request GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + goto goon;
>>> + }
>>> +
>>> + ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> + if (ret < 0) {
>>> + pr_err("%s, Failed to set input for GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> + if (irq < 0) {
>>> + ret = irq;
>>> + pr_err("Failed to set irq for %s\n",
>>> + GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + plat_dat = dev_get_platdata(&pdev->dev);
>>> + if (plat_dat)
>>> + plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> + else
>>> + pr_err("%s: plat_data is NULL\n", __func__);
The glue layer should never ever touch the platform data. If you need
to add interrupts for the phy, please do so in the stmmac driver proper,
and add the proper DT bindings/
>>> + }
>>> +
>>> +goon:
>>> + /*rmii or rgmii*/
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> + pr_info("%s: init for RGMII\n", __func__);
>>> + set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
>>> bsp_priv->rx_delay);
>>> + } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + pr_info("%s: init for RMII\n", __func__);
>>> + set_to_rmii(bsp_priv);
>>> + } else {
>>> + pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> + }
>>> +
>>> + bsp_priv->pdev = pdev;
>>> +
>>> + gmac_clk_init(bsp_priv);
>>> +
>>> + return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> + int ret;
>>> +
>>> + ret = phy_power_on(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = gmac_clk_enable(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *gmac = priv;
>>> +
>>> + phy_power_on(gmac, false);
>>> + gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> +
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> + set_rgmii_speed(bsp_priv, speed);
>>> + else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + set_rmii_speed(bsp_priv, speed);
>>> + else
>>> + pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> + .has_gmac = 1,
>>> + .fix_mac_speed = rk_fix_speed,
>>> + .setup = rk_gmac_setup,
>>> + .init = rk_gmac_init,
>>> + .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>>> 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>> static const struct of_device_id stmmac_dt_ids[] = {
>>> /* SoC specific glue layers should come before generic bindings
>>> */
>>> + { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>>
>> please name that rk3288_gmac_data [of course the other occurences too]
>> It makes it easier to see which soc it is meant for and it's also not save
>> to
>> assume that the next one will use the same register + bit positions in the
>> grf.
>>
>>
>>> { .compatible = "amlogic,meson6-dwmac", .data =
>>> &meson6_dwmac_data},
>>> { .compatible = "allwinner,sun7i-a20-gmac", .data =
>>> &sun7i_gmac_data},
>>> { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct platform_device
>>> *pdev) return -ENOMEM;
>>> }
>>>
>>> + pdev->dev.platform_data = plat_dat;
>>> +
When we re-did the DT layer for stmmac, there was consensus to _not_
use platform_data. Please do not use it.
>>> ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>> if (ret) {
>>> pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>>> 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>> extern const struct stmmac_of_data stih4xx_dwmac_data;
>>> extern const struct stmmac_of_data stid127_dwmac_data;
>>> extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>> #endif /* __STMMAC_PLATFORM_H__ */
>>
>>
>>
I may have repeated myself a few times. Please take a look at how
the other glue layers are written. This one is larger than it needs
to be.
Regards,
ChenYu
^ permalink raw reply
* [PATCH net v3 2/2] e100: Add netif_napi_del in the driver
From: Jia-Ju Bai @ 2014-12-25 2:02 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
In-Reply-To: <1419472956-6270-1-git-send-email-baijiaju1990@163.com>
The driver lacks netif_napi_del in the normal path and error path to
match the call of netif_napi_add in e100_probe.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e100.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 781065e..21c4d0f 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2985,6 +2985,7 @@ err_out_free_res:
err_out_disable_pdev:
pci_disable_device(pdev);
err_out_free_dev:
+ netif_napi_del(&nic->napi);
free_netdev(netdev);
return err;
}
@@ -2995,6 +2996,7 @@ static void e100_remove(struct pci_dev *pdev)
if (netdev) {
struct nic *nic = netdev_priv(netdev);
+ netif_napi_del(&nic->napi);
unregister_netdev(netdev);
e100_free(nic);
pci_iounmap(pdev, nic->csr);
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3 1/2] e100: Fix a null pointer deference in e100_probe
From: Jia-Ju Bai @ 2014-12-25 2:02 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
The driver lacks the check of nic->cbs_pool after pci_pool_create
in e100_probe. So when this function is failed, the null pointer
dereference occurs when pci_pool_alloc uses nic->cbs_pool in e100_alloc_cbs.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e100.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 781065e..ba1813f 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -2969,6 +2969,10 @@ static int e100_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
nic->params.cbs.max * sizeof(struct cb),
sizeof(u32),
0);
+ if (!nic->cbs_pool) {
+ err = -ENOMEM;
+ goto err_out_pool;
+ }
netif_info(nic, probe, nic->netdev,
"addr 0x%llx, irq %d, MAC addr %pM\n",
(unsigned long long)pci_resource_start(pdev, use_io ? 1 : 0),
@@ -2976,6 +2980,8 @@ static int e100_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
return 0;
+err_out_pool:
+ unregister_netdev(netdev);
err_out_free:
e100_free(nic);
err_out_iounmap:
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3] e1000: Add netif_napi_del in the driver
From: Jia-Ju Bai @ 2014-12-25 2:00 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
The driver lacks netif_napi_del in the normal path
and error path to match the call of netif_napi_add in e1000_probe.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e1000/e1000_main.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 24f3986..f6def7b 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -1004,7 +1004,7 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
/* make ready for any if (hw->...) below */
err = e1000_init_hw_struct(adapter, hw);
if (err)
- goto err_sw_init;
+ goto err_dma;
/* there is a workaround being applied below that limits
* 64-bit DMA addresses to 64-bit hardware. There are some
@@ -1239,8 +1239,9 @@ err_eeprom:
iounmap(hw->flash_address);
kfree(adapter->tx_ring);
kfree(adapter->rx_ring);
-err_dma:
err_sw_init:
+ netif_napi_del(&adapter->napi);
+err_dma:
err_mdio_ioremap:
iounmap(hw->ce4100_gbe_mdio_base_virt);
iounmap(hw->hw_addr);
@@ -1271,6 +1272,7 @@ static void e1000_remove(struct pci_dev *pdev)
e1000_down_and_stop(adapter);
e1000_release_manageability(adapter);
+ netif_napi_del(&adapter->napi);
unregister_netdev(netdev);
e1000_phy_hw_reset(hw);
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3 4/4] e1000e: Add pci_disable_pcie_error_reporting in error handling
From: Jia-Ju Bai @ 2014-12-25 1:57 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
In-Reply-To: <1419472623-6060-1-git-send-email-baijiaju1990@163.com>
The driver lacks pci_disable_pcie_error_reporting in error handling,
which should match pci_enable_pcie_error_reporting in e1000_probe.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 247335d..098c3c2 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7053,6 +7053,7 @@ err_flashmap:
err_ioremap:
free_netdev(netdev);
err_alloc_etherdev:
+ pci_disable_pcie_error_reporting(pdev);
pci_release_selected_regions(pdev,
pci_select_bars(pdev, IORESOURCE_MEM));
err_pci_reg:
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3 3/4] e1000e: Add netif_napi_del in the driver
From: Jia-Ju Bai @ 2014-12-25 1:57 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
In-Reply-To: <1419472623-6060-1-git-send-email-baijiaju1990@163.com>
The driver lacks netif_napi_del in the normal path
and error path to match the call of netif_napi_add in e1000_probe.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 247335d..c5f0afc 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -7045,6 +7045,7 @@ err_hw_init:
kfree(adapter->tx_ring);
kfree(adapter->rx_ring);
err_sw_init:
+ netif_napi_del(&adapter->napi);
if (adapter->hw.flash_address)
iounmap(adapter->hw.flash_address);
e1000e_reset_interrupt_capability(adapter);
@@ -7103,6 +7104,7 @@ static void e1000_remove(struct pci_dev *pdev)
/* Don't lie to e1000_close() down the road. */
if (!down)
clear_bit(__E1000_DOWN, &adapter->state);
+ netif_napi_del(&adapter->napi);
unregister_netdev(netdev);
if (pci_dev_run_wake(pdev))
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3 2/4] e1000e: Add pm_qos_remove_request in error handling
From: Jia-Ju Bai @ 2014-12-25 1:57 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
In-Reply-To: <1419472623-6060-1-git-send-email-baijiaju1990@163.com>
The driver lacks pm_qos_remove_request in error handling,
which should match pm_qos_add_request in e1000_open.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 247335d..71bc244 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -4430,6 +4430,7 @@ static int e1000_open(struct net_device *netdev)
return 0;
err_req_irq:
+ pm_qos_remove_request(&adapter->netdev->pm_qos_req);
e1000e_release_hw_control(adapter);
e1000_power_down_phy(adapter);
e1000e_free_rx_resources(adapter->rx_ring);
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* [PATCH net v3 1/4] e1000e: Fix a null deference in e1000_open
From: Jia-Ju Bai @ 2014-12-25 1:57 UTC (permalink / raw)
To: jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose, matthew.vick,
john.ronciak, mitch.a.williams
Cc: e1000-devel, netdev, Jia-Ju Bai, linux.nics
The function vzalloc is called by e1000e_setup_rx_resources (in
e1000_open) when initializing the ethernet card driver. But when vzalloc is
failed, "err" segment in e1000e_setup_rx_resources is executed to return,
and then e1000e_free_tx_resources in "err_setup_rx" segment is executed to
halt e1000_open. However, "writel(0, tx_ring->head)" statement in
e1000_clean_tx_ring in e1000e_free_tx_resources will cause system crash,
because "tx_ring->head" is not assigned the value. In the code,
"tx_ring->head" is initialized in e1000_configure_tx in e1000_configure
after the e1000e_setup_rx_resources.
This patch fixes this problem, and it has been tested on the hardware.
Signed-off-by: Jia-Ju Bai <baijiaju1990@163.com>
---
drivers/net/ethernet/intel/e1000e/netdev.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 247335d..728328b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -1737,6 +1737,8 @@ static void e1000_clean_rx_ring(struct e1000_ring *rx_ring)
rx_ring->next_to_use = 0;
adapter->flags2 &= ~FLAG2_IS_DISCARDING;
+ if (!rx_ring->head)
+ return;
writel(0, rx_ring->head);
if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA)
e1000e_update_rdt_wa(rx_ring, 0);
@@ -2444,6 +2446,8 @@ static void e1000_clean_tx_ring(struct e1000_ring *tx_ring)
tx_ring->next_to_use = 0;
tx_ring->next_to_clean = 0;
+ if (!tx_ring->head)
+ return;
writel(0, tx_ring->head);
if (adapter->flags2 & FLAG2_PCIM2PCI_ARBITER_WA)
e1000e_update_tdt_wa(tx_ring, 0);
@@ -4357,6 +4361,9 @@ static int e1000_open(struct net_device *netdev)
netif_carrier_off(netdev);
+ adapter->tx_ring->head = NULL;
+ adapter->rx_ring->head = NULL;
+
/* allocate transmit descriptors */
err = e1000e_setup_tx_resources(adapter->tx_ring);
if (err)
--
1.7.9.5
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired
^ permalink raw reply related
* Re: [PATCH v2 1/6] GMAC: add driver for Rockchip RK3288 SoCs integrated GMAC
From: Roger @ 2014-12-25 0:46 UTC (permalink / raw)
To: Heiko Stübner
Cc: peppe.cavallaro, netdev, linux-kernel, linux-rockchip, kever.yang,
eddie.cai
In-Reply-To: <547EC24E.6050905@rock-chips.com>
Hi! Heiko
Any suggestion?
On 2014/12/3 15:57, Roger wrote:
> Hi! Heiko
>
> about regulator, power gpio, reset gpio and irq gpio
> please refer to my comment inline, tks.
>
> On 2014/12/2 7:44, Heiko Stübner wrote:
>> Hi Roger,
>>
>> the comments inline are a rough first review. I hope to get a clearer
>> picture
>> for the stuff I'm not sure about in v3 once the big issues are fixed.
>>
>>
>> Am Donnerstag, 27. November 2014, 10:52:08 schrieb Roger Chen:
>>> This driver is based on stmmac driver.
>>>
>>> modification based on Giuseppe CAVALLARO's suggestion:
>>> 1. use BIT()
>>>
>>> > +/*RK3288_GRF_SOC_CON3*/
>>> > +#define GMAC_TXCLK_DLY_ENABLE ((0x4000 << 16) | (0x4000))
>>> > +#define GMAC_TXCLK_DLY_DISABLE ((0x4000 << 16) | (0x0000))
>>>
>>> ...
>>>
>>> why do not use BIT and BIT_MASK where possible?
>>>
>>> ===>after modification:
>>>
>>> #define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> #define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> #define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> #define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> ...
>>> 2.
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_PHY_INTF_SEL_RGMII);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> > + GMAC_RMII_MODE_CLR);
>>>
>>> maybe you could perform just one write unless there is some HW
>>> constraint.
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>>
>>> 3. use macros
>>>
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E,
>>> 0xFFFFFFFF);
>>> > + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E,
>>> > + 0x3<<2<<16 | 0x3<<2);
>>>
>>> pls use macros, these shift sequence is really help to decode
>>>
>>> ===>after modification:
>>>
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>>>
>>> 4. remove grf fail check in rk_gmac_setup()
>>>
>>> > + if (IS_ERR(bsp_priv->grf))
>>> > + dev_err(&pdev->dev, "Missing rockchip,grf property\n");
>>>
>>> I wonder if you can fail on here and save all the check in
>>> set_rgmii_speed etc.
>>> Maybe this can be considered a mandatory property for the
>>> glue-logic.
>>>
>>> 5. remove .tx_coe=1
>>>
>>> > +const struct stmmac_of_data rk_gmac_data = {
>>> > + .has_gmac = 1,
>>> > + .tx_coe = 1,
>>>
>>> FYI, on new gmac there is the HW capability register to dinamically
>>> provide you if coe is supported.
>>>
>>> IMO you should add the OF "compatible" string and in case of mac
>>> newer than the 3.50a you can remove coe.
>> changelogs like these, should be compact and also not be in the
>> commit message
>> itself, but in the "comment"-section below the "---" and before the
>> diffstat.
>>
>>
>>> Signed-off-by: Roger Chen <roger.chen@rock-chips.com>
>>> ---
>> changelog here ... the commonly used pattern is something like
>>
>> changes since v2:
>> - ...
>> - ...
>>
>> changes since v1:
>> - ...
>>
>>> drivers/net/ethernet/stmicro/stmmac/Makefile | 2 +-
>>> drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 636
>>> ++++++++++++++++++++
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.c |
>>> 3 +
>>> .../net/ethernet/stmicro/stmmac/stmmac_platform.h | 1 +
>>> 4 files changed, 641 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> b/drivers/net/ethernet/stmicro/stmmac/Makefile index ac4d562..73c2715
>>> 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/Makefile
>>> @@ -6,7 +6,7 @@ stmmac-objs:= stmmac_main.o stmmac_ethtool.o
>>> stmmac_mdio.o
>>> ring_mode.o \
>>>
>>> obj-$(CONFIG_STMMAC_PLATFORM) += stmmac-platform.o
>>> stmmac-platform-objs:= stmmac_platform.o dwmac-meson.o
>>> dwmac-sunxi.o \
>>> - dwmac-sti.o dwmac-socfpga.o
>>> + dwmac-sti.o dwmac-socfpga.o dwmac-rk.o
>>>
>>> obj-$(CONFIG_STMMAC_PCI) += stmmac-pci.o
>>> stmmac-pci-objs:= stmmac_pci.o
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c new file mode 100644
>>> index 0000000..870563f
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>>> @@ -0,0 +1,636 @@
>>> +/**
>>> + * dwmac-rk.c - Rockchip RK3288 DWMAC specific glue layer
>>> + *
>>> + * Copyright (C) 2014 Chen-Zhi (Roger Chen)
>>> + *
>>> + * Chen-Zhi (Roger Chen) <roger.chen@rock-chips.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License as
>>> published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>> + * GNU General Public License for more details.
>>> + */
>>> +
>>> +#include <linux/stmmac.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/clk.h>
>>> +#include <linux/phy.h>
>>> +#include <linux/of_net.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +struct rk_priv_data {
>>> + struct platform_device *pdev;
>>> + int phy_iface;
>>> + bool power_ctrl_by_pmu;
>>> + char pmu_regulator[32];
>>> + int pmu_enable_level;
>>> +
>>> + int power_io;
>>> + int power_io_level;
>>> + int reset_io;
>>> + int reset_io_level;
>>> + int phyirq_io;
>>> + int phyirq_io_level;
>>> +
>>> + bool clk_enabled;
>>> + bool clock_input;
>>> +
>>> + struct clk *clk_mac;
>>> + struct clk *clk_mac_pll;
>>> + struct clk *gmac_clkin;
>>> + struct clk *mac_clk_rx;
>>> + struct clk *mac_clk_tx;
>>> + struct clk *clk_mac_ref;
>>> + struct clk *clk_mac_refout;
>>> + struct clk *aclk_mac;
>>> + struct clk *pclk_mac;
>>> +
>>> + int tx_delay;
>>> + int rx_delay;
>>> +
>>> + struct regmap *grf;
>>> +};
>>> +
>>> +#define RK3288_GRF_SOC_CON1 0x0248
>>> +#define RK3288_GRF_SOC_CON3 0x0250
>>> +#define RK3288_GRF_GPIO3D_E 0x01ec
>>> +#define RK3288_GRF_GPIO4A_E 0x01f0
>>> +#define RK3288_GRF_GPIO4B_E 0x01f4
>> here you're using a space instead of a tab, please select one pattern
>> either
>> tabs or space but do not mix them.
>>
>>
>>> +#define GPIO3D_2MA 0xFFFF0000
>>> +#define GPIO3D_4MA 0xFFFF5555
>>> +#define GPIO3D_8MA 0xFFFFAAAA
>>> +#define GPIO3D_12MA 0xFFFFFFFF
>>> +
>>> +#define GPIO4A_2MA 0xFFFF0000
>>> +#define GPIO4A_4MA 0xFFFF5555
>>> +#define GPIO4A_8MA 0xFFFFAAAA
>>> +#define GPIO4A_12MA 0xFFFFFFFF
>> see comment about pin settings below
>>
>>
>>> +
>>> +#define GRF_BIT(nr) (BIT(nr) | BIT(nr+16))
>>> +#define GRF_CLR_BIT(nr) (BIT(nr+16))
>>> +
>>> +#define GPIO4B_2_2MA (GRF_CLR_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_4MA (GRF_BIT(2) | GRF_CLR_BIT(3))
>>> +#define GPIO4B_2_8MA (GRF_CLR_BIT(2) | GRF_BIT(3))
>>> +#define GPIO4B_2_12MA (GRF_BIT(2) | GRF_BIT(3))
>>> +
>>> +/*RK3288_GRF_SOC_CON1*/
>>> +#define GMAC_PHY_INTF_SEL_RGMII (GRF_BIT(6) | GRF_CLR_BIT(7) |
>>> GRF_CLR_BIT(8))
>>> +#define GMAC_PHY_INTF_SEL_RMII (GRF_CLR_BIT(6) |
>>> GRF_CLR_BIT(7) | GRF_BIT(8))
>>> +#define GMAC_FLOW_CTRL GRF_BIT(9)
>>> +#define GMAC_FLOW_CTRL_CLR GRF_CLR_BIT(9)
>>> +#define GMAC_SPEED_10M GRF_CLR_BIT(10)
>>> +#define GMAC_SPEED_100M GRF_BIT(10)
>>> +#define GMAC_RMII_CLK_25M GRF_BIT(11)
>>> +#define GMAC_RMII_CLK_2_5M GRF_CLR_BIT(11)
>>> +#define GMAC_CLK_125M (GRF_CLR_BIT(12) | GRF_CLR_BIT(13))
>>> +#define GMAC_CLK_25M (GRF_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_CLK_2_5M (GRF_CLR_BIT(12) | GRF_BIT(13))
>>> +#define GMAC_RMII_MODE GRF_BIT(14)
>>> +#define GMAC_RMII_MODE_CLR GRF_CLR_BIT(14)
>>> +
>>> +/*RK3288_GRF_SOC_CON3*/
>>> +#define GMAC_TXCLK_DLY_ENABLE GRF_BIT(14)
>>> +#define GMAC_TXCLK_DLY_DISABLE GRF_CLR_BIT(14)
>>> +#define GMAC_RXCLK_DLY_ENABLE GRF_BIT(15)
>>> +#define GMAC_RXCLK_DLY_DISABLE GRF_CLR_BIT(15)
>>> +#define GMAC_CLK_RX_DL_CFG(val) ((0x7F<<7<<16) | (val<<7))
>>> +#define GMAC_CLK_TX_DL_CFG(val) ((0x7F<<16) | (val))
>> again mixed tabs and spaces as delimiters.
>>
>> Also the _CFG macros are not well abstracted. You could take a look
>> at the
>> HIWORD_UPDATE macro in drivers/clk/rockchip/clk.h:
>>
>> #define GMAC_CLK_DL_MASK 0x7f
>> #define GMAC_CLK_RX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 7)
>> #define GMAC_CLK_TX_DL_CFG(val) HIWORD_UPDATE(val, GMAC_CLK_DL_MASK, 0)
>>
>>
>>> +
>>> +static void set_to_rgmii(struct rk_priv_data *bsp_priv,
>>> + int tx_delay, int rx_delay)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RGMII | GMAC_RMII_MODE_CLR);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
>>> + GMAC_RXCLK_DLY_ENABLE | GMAC_TXCLK_DLY_ENABLE |
>>> + GMAC_CLK_RX_DL_CFG(rx_delay) |
>>> + GMAC_CLK_TX_DL_CFG(tx_delay));
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO3D_E, GPIO3D_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4A_E, GPIO4A_12MA);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_GPIO4B_E, GPIO4B_2_12MA);
>> please don't write to parts controlled by other drivers - here the drive
>> strength settings of pins is controlled by the pinctrl driver.
>> Instead you can
>> just set the drive-strength in the pinctrl settings.
>>
>>
>>> +
>>> + pr_debug("%s: tx delay=0x%x; rx delay=0x%x;\n",
>>> + __func__, tx_delay, rx_delay);
>>> +}
>>> +
>>> +static void set_to_rmii(struct rk_priv_data *bsp_priv)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>> you have a device-reference in rk_priv_data, so you could use dev_err
>> here.
>> Same for all other pr_err/pr_debug/pr_* calls in this file.
>>
>>
>>> + return;
>>> + }
>>> +
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_PHY_INTF_SEL_RMII);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_MODE);
>> these two could be combined?
>>
>>
>>> +}
>>> +
>>> +static void set_rgmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_2_5M);
>>> + else if (speed == 100)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_25M);
>>> + else if (speed == 1000)
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> GMAC_CLK_125M);
>>> + else
>>> + pr_err("unknown speed value for RGMII! speed=%d", speed);
>>> +}
>>> +
>>> +static void set_rmii_speed(struct rk_priv_data *bsp_priv, int speed)
>>> +{
>>> + if (IS_ERR(bsp_priv->grf)) {
>>> + pr_err("%s: Missing rockchip,grf property\n", __func__);
>>> + return;
>>> + }
>>> +
>>> + if (speed == 10) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_2_5M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_10M);
>> combine into one write?
>>
>>
>>> + } else if (speed == 100) {
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_RMII_CLK_25M);
>>> + regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON1,
>>> + GMAC_SPEED_100M);
>> combine into one write?
>>
>>
>>> + } else {
>>> + pr_err("unknown speed value for RMII! speed=%d", speed);
>>> + }
>>> +}
>>> +
>>> +#define MAC_CLK_RX "mac_clk_rx"
>>> +#define MAC_CLK_TX "mac_clk_tx"
>>> +#define CLK_MAC_REF "clk_mac_ref"
>>> +#define CLK_MAC_REF_OUT "clk_mac_refout"
>>> +#define CLK_MAC_PLL "clk_mac_pll"
>>> +#define ACLK_MAC "aclk_mac"
>>> +#define PCLK_MAC "pclk_mac"
>>> +#define MAC_CLKIN "ext_gmac"
>>> +#define CLK_MAC "stmmaceth"
>> why the need to extra constants for the clock names and not use the
>> real names
>> directly like most other drivers do?
>>
>>
>>> +
>>> +static int gmac_clk_init(struct rk_priv_data *bsp_priv)
>>> +{
>>> + struct device *dev = &bsp_priv->pdev->dev;
>>> +
>>> + bsp_priv->clk_enabled = false;
>>> +
>>> + bsp_priv->mac_clk_rx = clk_get(dev, MAC_CLK_RX);
>>> + if (IS_ERR(bsp_priv->mac_clk_rx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_RX);
>>> +
>>> + bsp_priv->mac_clk_tx = clk_get(dev, MAC_CLK_TX);
>>> + if (IS_ERR(bsp_priv->mac_clk_tx))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLK_TX);
>>> +
>>> + bsp_priv->clk_mac_ref = clk_get(dev, CLK_MAC_REF);
>>> + if (IS_ERR(bsp_priv->clk_mac_ref))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF);
>>> +
>>> + bsp_priv->clk_mac_refout = clk_get(dev, CLK_MAC_REF_OUT);
>>> + if (IS_ERR(bsp_priv->clk_mac_refout))
>>> + pr_warn("%s: warning:cannot get clock %s\n",
>>> + __func__, CLK_MAC_REF_OUT);
>>> +
>>> + bsp_priv->aclk_mac = clk_get(dev, ACLK_MAC);
>>> + if (IS_ERR(bsp_priv->aclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, ACLK_MAC);
>>> +
>>> + bsp_priv->pclk_mac = clk_get(dev, PCLK_MAC);
>>> + if (IS_ERR(bsp_priv->pclk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, PCLK_MAC);
>>> +
>>> + bsp_priv->clk_mac_pll = clk_get(dev, CLK_MAC_PLL);
>>> + if (IS_ERR(bsp_priv->clk_mac_pll))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC_PLL);
>>> +
>>> + bsp_priv->gmac_clkin = clk_get(dev, MAC_CLKIN);
>>> + if (IS_ERR(bsp_priv->gmac_clkin))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, MAC_CLKIN);
>>> +
>>> + bsp_priv->clk_mac = clk_get(dev, CLK_MAC);
>>> + if (IS_ERR(bsp_priv->clk_mac))
>>> + pr_warn("%s: warning: cannot get clock %s\n",
>>> + __func__, CLK_MAC);
>> there is not clk_put in the _remove case ... maybe you could simply use
>> devm_clk_get here so that all clocks are put on device removal.
>>
>> Also you're warning on every missing clock. Below it looks like you
>> need a
>> different set of them for rgmii and rmii, so maybe you should simply
>> error out
>> when core clocks for the selected phy-mode are missing.
>>
>>
>>> +
>>> + if (bsp_priv->clock_input) {
>>> + pr_info("%s: clock input from PHY\n", __func__);
>>> + } else {
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + clk_set_rate(bsp_priv->clk_mac_pll, 50000000);
>>> +
>>> + clk_set_parent(bsp_priv->clk_mac, bsp_priv->clk_mac_pll);
>> why the explicit reparenting. The common clock-framework is
>> intelligent enough
>> to select the best suitable parent.
>>
>> In general I'm thinking the clock-handling inside this driver should be
>> simplyfied, as the common-clock framework can handle most cases
>> itself. I.e. if
>> a 125MHz external clock is present and so on. But haven't looked to
>> deep yet.
>>
>>
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int phy_iface = phy_iface = bsp_priv->phy_iface;
>>> +
>>> + if (enable) {
>>> + if (!bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_prepare_enable(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_prepare_enable(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_prepare_enable(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_prepare_enable(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_prepare_enable(bsp_priv->mac_clk_tx);
>>> +
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_prepare_enable(bsp_priv->clk_mac);
>>> + */
>>> + mdelay(5);
>>> + bsp_priv->clk_enabled = true;
>>> + }
>>> + } else {
>>> + if (bsp_priv->clk_enabled) {
>>> + if (phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + if (!IS_ERR(bsp_priv->mac_clk_rx))
>>> + clk_disable_unprepare(
>>> + bsp_priv->mac_clk_rx);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_ref))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_ref);
>>> +
>>> + if (!IS_ERR(bsp_priv->clk_mac_refout))
>>> + clk_disable_unprepare(
>>> + bsp_priv->clk_mac_refout);
>>> + }
>>> +
>>> + if (!IS_ERR(bsp_priv->aclk_mac))
>>> + clk_disable_unprepare(bsp_priv->aclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->pclk_mac))
>>> + clk_disable_unprepare(bsp_priv->pclk_mac);
>>> +
>>> + if (!IS_ERR(bsp_priv->mac_clk_tx))
>>> + clk_disable_unprepare(bsp_priv->mac_clk_tx);
>>> + /**
>>> + * if (!IS_ERR(bsp_priv->clk_mac))
>>> + * clk_disable_unprepare(bsp_priv->clk_mac);
>>> + */
>>> + bsp_priv->clk_enabled = false;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_pmu(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + struct regulator *ldo;
>>> + char *ldostr = bsp_priv->pmu_regulator;
>>> + int ret;
>>> +
>>> + if (!ldostr) {
>>> + pr_err("%s: no ldo found\n", __func__);
>>> + return -1;
>>> + }
>>> +
>>> + ldo = regulator_get(NULL, ldostr);
>>> + if (!ldo) {
>>> + pr_err("\n%s get ldo %s failed\n", __func__, ldostr);
>>> + } else {
>>> + if (enable) {
>>> + if (!regulator_is_enabled(ldo)) {
>>> + regulator_set_voltage(ldo, 3300000, 3300000);
>>> + ret = regulator_enable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to enable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn on ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is enabled before enable", ldostr);
>>> + }
>>> + } else {
>>> + if (regulator_is_enabled(ldo)) {
>>> + ret = regulator_disable(ldo);
>>> + if (ret != 0)
>>> + pr_err("%s: fail to disable %s\n",
>>> + __func__, ldostr);
>>> + else
>>> + pr_info("turn off ldo done.\n");
>>> + } else {
>>> + pr_warn("%s is disabled before disable",
>>> + ldostr);
>>> + }
>>> + }
>>> + regulator_put(ldo);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int power_on_by_gpio(struct rk_priv_data *bsp_priv, bool
>>> enable)
>>> +{
>>> + if (enable) {
>>> + /*power on*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + bsp_priv->power_io_level);
>>> + } else {
>>> + /*power off*/
>>> + if (gpio_is_valid(bsp_priv->power_io))
>>> + gpio_direction_output(bsp_priv->power_io,
>>> + !bsp_priv->power_io_level);
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>> +{
>>> + int ret = -1;
>>> +
>>> + pr_info("Ethernet PHY power %s\n", enable == 1 ? "on" : "off");
>>> +
>>> + if (bsp_priv->power_ctrl_by_pmu)
>>> + ret = power_on_by_pmu(bsp_priv, enable);
>>> + else
>>> + ret = power_on_by_gpio(bsp_priv, enable);
>> this looks wrong. This should always be a regulator. Even a regulator
>> + switch
>> controlled by a gpio can still be modelled as regulator, so that you
>> don't
>> need this switch and assorted special handling - so just use the
>> regulator API
>>
> In some case, it would be a switching circuit to control the power for
> PHY.
> All I need to do is to control a GPIO to make switch on/off. So...
>>> +
>>> + if (enable) {
>>> + /*reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + mdelay(5);
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + !bsp_priv->reset_io_level);
>>> + }
>>> + mdelay(30);
>>> +
>>> + } else {
>>> + /*pull down reset*/
>>> + if (gpio_is_valid(bsp_priv->reset_io)) {
>>> + gpio_direction_output(bsp_priv->reset_io,
>>> + bsp_priv->reset_io_level);
>>> + }
>>> + }
>> I'm not sure yet if it would be better to use the reset framework for
>> this.
>> While it says it is also meant for reset-gpios, there does not seem a
>> driver
>> for this to exist yet.
>>
> What should I do?
>>
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +#define GPIO_PHY_POWER "gmac_phy_power"
>>> +#define GPIO_PHY_RESET "gmac_phy_reset"
>>> +#define GPIO_PHY_IRQ "gmac_phy_irq"
>> again I don't understand why these constants are necessary
>>
>>> +
>>> +static void *rk_gmac_setup(struct platform_device *pdev)
>>> +{
>>> + struct rk_priv_data *bsp_priv;
>>> + struct device *dev = &pdev->dev;
>>> + enum of_gpio_flags flags;
>>> + int ret;
>>> + const char *strings = NULL;
>>> + int value;
>>> + int irq;
>>> +
>>> + bsp_priv = devm_kzalloc(dev, sizeof(*bsp_priv), GFP_KERNEL);
>>> + if (!bsp_priv)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>> +
>>> + ret = of_property_read_string(dev->of_node, "pmu_regulator",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_regulator.\n",
>>> __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: ethernet phy power controlled by pmu(%s).\n",
>>> + __func__, strings);
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + strcpy(bsp_priv->pmu_regulator, strings);
>>> + }
>> There is a generic regulator-dt-binding for regulator-consumers
>> available
>> which you should of course use.
>>
> The same explanation as above
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "pmu_enable_level",
>>> &value);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: pmu_enable_level.\n",
>>> + __func__);
>>> + bsp_priv->power_ctrl_by_pmu = false;
>>> + } else {
>>> + pr_info("%s: PHY power controlled by pmu(level = %s).\n",
>>> + __func__, (value == 1) ? "HIGH" : "LOW");
>>> + bsp_priv->power_ctrl_by_pmu = true;
>>> + bsp_priv->pmu_enable_level = value;
>>> + }
>> What is this used for? Enabling should of course be done via
>> regulator_enable
>> and disabling using regulator_disable.
>>
>>
>>> +
>>> + ret = of_property_read_string(dev->of_node, "clock_in_out",
>>> &strings);
>>> + if (ret) {
>>> + pr_err("%s: Can not read property: clock_in_out.\n",
>>> __func__);
>>> + bsp_priv->clock_input = true;
>>> + } else {
>>> + pr_info("%s: clock input or output? (%s).\n",
>>> + __func__, strings);
>>> + if (!strcmp(strings, "input"))
>>> + bsp_priv->clock_input = true;
>>> + else
>>> + bsp_priv->clock_input = false;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->tx_delay = 0x30;
>>> + pr_err("%s: Can not read property: tx_delay.", __func__);
>>> + pr_err("%s: set tx_delay to 0x%x\n",
>>> + __func__, bsp_priv->tx_delay);
>>> + } else {
>>> + pr_info("%s: TX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->tx_delay = value;
>>> + }
>>> +
>>> + ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>>> + if (ret) {
>>> + bsp_priv->rx_delay = 0x10;
>>> + pr_err("%s: Can not read property: rx_delay.", __func__);
>>> + pr_err("%s: set rx_delay to 0x%x\n",
>>> + __func__, bsp_priv->rx_delay);
>>> + } else {
>>> + pr_info("%s: RX delay(0x%x).\n", __func__, value);
>>> + bsp_priv->rx_delay = value;
>>> + }
>>> +
>>> + bsp_priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>> + "rockchip,grf");
>>> + bsp_priv->phyirq_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "phyirq-gpio", 0, &flags);
>>> + bsp_priv->phyirq_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->reset_io =
>>> + of_get_named_gpio_flags(dev->of_node,
>>> + "reset-gpio", 0, &flags);
>>> + bsp_priv->reset_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + bsp_priv->power_io =
>>> + of_get_named_gpio_flags(dev->of_node, "power-gpio", 0,
>>> &flags);
>>> + bsp_priv->power_io_level = (flags & OF_GPIO_ACTIVE_LOW) ? 0 : 1;
>>> +
>>> + /*power*/
>>> + if (!gpio_is_valid(bsp_priv->power_io)) {
>>> + pr_err("%s: Failed to get GPIO %s.\n",
>>> + __func__, "power-gpio");
>>> + } else {
>>> + ret = gpio_request(bsp_priv->power_io, GPIO_PHY_POWER);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_POWER);
>>> + }
>> When everything power-related is handled using the regulator api, you
>> don't
>> need this
> The same explanation as above
>>
>>> +
>>> + if (!gpio_is_valid(bsp_priv->reset_io)) {
>>> + pr_err("%s: ERROR: Get reset-gpio failed.\n", __func__);
>>> + } else {
>>> + ret = gpio_request(bsp_priv->reset_io, GPIO_PHY_RESET);
>>> + if (ret)
>>> + pr_err("%s: ERROR: Failed to request GPIO %s.\n",
>>> + __func__, GPIO_PHY_RESET);
>>> + }
>>> +
>>> + if (bsp_priv->phyirq_io > 0) {
>> This is more for my understanding: why does the mac driver need to
>> handle the
>> phy interrupt - but I might be overlooking something.
>>
> phy interrupt is not mandatory. In most of the time, in order to
> find something happen in PHY, for example,
> link is up or down, we just use polling method to read the phy's
> register in a timer.
> Buf if phy interrupt is in use, when link is up or down, phy
> interrupt pin will be assert to inform the CPU.
> I just implement the driver for phy interrupt gpio, not enable it as
> default.
>
>>> + struct plat_stmmacenet_data *plat_dat;
>>> +
>>> + pr_info("PHY irq in use\n");
>>> + ret = gpio_request(bsp_priv->phyirq_io, GPIO_PHY_IRQ);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Failed to request GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + goto goon;
>>> + }
>>> +
>>> + ret = gpio_direction_input(bsp_priv->phyirq_io);
>>> + if (ret < 0) {
>>> + pr_err("%s, Failed to set input for GPIO %s\n",
>>> + __func__, GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + irq = gpio_to_irq(bsp_priv->phyirq_io);
>>> + if (irq < 0) {
>>> + ret = irq;
>>> + pr_err("Failed to set irq for %s\n",
>>> + GPIO_PHY_IRQ);
>>> + gpio_free(bsp_priv->phyirq_io);
>>> + goto goon;
>>> + }
>>> +
>>> + plat_dat = dev_get_platdata(&pdev->dev);
>>> + if (plat_dat)
>>> + plat_dat->mdio_bus_data->probed_phy_irq = irq;
>>> + else
>>> + pr_err("%s: plat_data is NULL\n", __func__);
>>> + }
>>> +
>>> +goon:
>>> + /*rmii or rgmii*/
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII) {
>>> + pr_info("%s: init for RGMII\n", __func__);
>>> + set_to_rgmii(bsp_priv, bsp_priv->tx_delay,
>>> bsp_priv->rx_delay);
>>> + } else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII) {
>>> + pr_info("%s: init for RMII\n", __func__);
>>> + set_to_rmii(bsp_priv);
>>> + } else {
>>> + pr_err("%s: ERROR: NO interface defined!\n", __func__);
>>> + }
>>> +
>>> + bsp_priv->pdev = pdev;
>>> +
>>> + gmac_clk_init(bsp_priv);
>>> +
>>> + return bsp_priv;
>>> +}
>>> +
>>> +static int rk_gmac_init(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> + int ret;
>>> +
>>> + ret = phy_power_on(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = gmac_clk_enable(bsp_priv, true);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void rk_gmac_exit(struct platform_device *pdev, void *priv)
>>> +{
>>> + struct rk_priv_data *gmac = priv;
>>> +
>>> + phy_power_on(gmac, false);
>>> + gmac_clk_enable(gmac, false);
>>> +}
>>> +
>>> +static void rk_fix_speed(void *priv, unsigned int speed)
>>> +{
>>> + struct rk_priv_data *bsp_priv = priv;
>>> +
>>> + if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RGMII)
>>> + set_rgmii_speed(bsp_priv, speed);
>>> + else if (bsp_priv->phy_iface == PHY_INTERFACE_MODE_RMII)
>>> + set_rmii_speed(bsp_priv, speed);
>>> + else
>>> + pr_err("unsupported interface %d", bsp_priv->phy_iface);
>>> +}
>>> +
>>> +const struct stmmac_of_data rk_gmac_data = {
>>> + .has_gmac = 1,
>>> + .fix_mac_speed = rk_fix_speed,
>>> + .setup = rk_gmac_setup,
>>> + .init = rk_gmac_init,
>>> + .exit = rk_gmac_exit,
>>> +};
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index
>>> 15814b7..b4dee96 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
>>> @@ -33,6 +33,7 @@
>>>
>>> static const struct of_device_id stmmac_dt_ids[] = {
>>> /* SoC specific glue layers should come before generic
>>> bindings */
>>> + { .compatible = "rockchip,rk3288-gmac", .data = &rk_gmac_data},
>> please name that rk3288_gmac_data [of course the other occurences too]
>> It makes it easier to see which soc it is meant for and it's also not
>> save to
>> assume that the next one will use the same register + bit positions
>> in the
>> grf.
>>
>>
>>> { .compatible = "amlogic,meson6-dwmac", .data =
>>> &meson6_dwmac_data},
>>> { .compatible = "allwinner,sun7i-a20-gmac", .data =
>>> &sun7i_gmac_data},
>>> { .compatible = "st,stih415-dwmac", .data = &stih4xx_dwmac_data},
>>> @@ -291,6 +292,8 @@ static int stmmac_pltfr_probe(struct
>>> platform_device
>>> *pdev) return -ENOMEM;
>>> }
>>>
>>> + pdev->dev.platform_data = plat_dat;
>>> +
>>> ret = stmmac_probe_config_dt(pdev, plat_dat, &mac);
>>> if (ret) {
>>> pr_err("%s: main dt probe failed", __func__);
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h index
>>> 25dd1f7..32a0516 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.h
>>> @@ -24,5 +24,6 @@ extern const struct stmmac_of_data sun7i_gmac_data;
>>> extern const struct stmmac_of_data stih4xx_dwmac_data;
>>> extern const struct stmmac_of_data stid127_dwmac_data;
>>> extern const struct stmmac_of_data socfpga_gmac_data;
>>> +extern const struct stmmac_of_data rk_gmac_data;
>>>
>>> #endif /* __STMMAC_PLATFORM_H__ */
>>
>>
>
^ permalink raw reply
* [PATCH v2 4/4] can: kvaser_usb: Add support for the Usbcan-II family
From: Ahmed S. Darwish @ 2014-12-25 0:04 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Greg KH, Linux-CAN, netdev, LKML
In-Reply-To: <20141225000256.GB24302@vivalin-002>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
CAN to USB interfaces sold by the Swedish manufacturer Kvaser are
divided into two major families: 'Leaf', and 'UsbcanII'. From an
Operating System perspective, the firmware of both families behave
in a not too drastically different fashion.
This patch adds support for the UsbcanII family of devices to the
current Kvaser Leaf-only driver.
CAN frames sending, receiving, and error handling paths has been
tested using the dual-channel "Kvaser USBcan II HS/LS" dongle. It
should also work nicely with other products in the same category.
List of new devices supported by this driver update:
- Kvaser USBcan II HS/HS
- Kvaser USBcan II HS/LS
- Kvaser USBcan Rugged ("USBcan Rev B")
- Kvaser Memorator HS/HS
- Kvaser Memorator HS/LS
- Scania VCI2 (if you have the Kvaser logo on top)
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/Kconfig | 8 +-
drivers/net/can/usb/kvaser_usb.c | 627 +++++++++++++++++++++++++++++++--------
2 files changed, 510 insertions(+), 125 deletions(-)
** V2 Changelog:
- Update Kconfig entries
- Use actual number of CAN channels (instead of max) where appropriate
- Rebase over a new set of UsbcanII-independent driver fixes
diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index a77db919..f6f5500 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig
@@ -25,7 +25,7 @@ config CAN_KVASER_USB
tristate "Kvaser CAN/USB interface"
---help---
This driver adds support for Kvaser CAN/USB devices like Kvaser
- Leaf Light.
+ Leaf Light and Kvaser USBcan II.
The driver provides support for the following devices:
- Kvaser Leaf Light
@@ -46,6 +46,12 @@ config CAN_KVASER_USB
- Kvaser USBcan R
- Kvaser Leaf Light v2
- Kvaser Mini PCI Express HS
+ - Kvaser USBcan II HS/HS
+ - Kvaser USBcan II HS/LS
+ - Kvaser USBcan Rugged ("USBcan Rev B")
+ - Kvaser Memorator HS/HS
+ - Kvaser Memorator HS/LS
+ - Scania VCI2 (if you have the Kvaser logo on top)
If unsure, say N.
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 2791501..50da317 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -6,10 +6,12 @@
* Parts of this driver are based on the following:
* - Kvaser linux leaf driver (version 4.78)
* - CAN driver for esd CAN-USB/2
+ * - Kvaser linux usbcanII driver (version 5.3)
*
* Copyright (C) 2002-2006 KVASER AB, Sweden. All rights reserved.
* Copyright (C) 2010 Matthias Fuchs <matthias.fuchs@esd.eu>, esd gmbh
* Copyright (C) 2012 Olivier Sobrie <olivier@sobrie.be>
+ * Copyright (C) 2014 Valeo Corporation
*/
#include <linux/completion.h>
@@ -21,6 +23,18 @@
#include <linux/can/dev.h>
#include <linux/can/error.h>
+/*
+ * Kvaser USB CAN dongles are divided into two major families:
+ * - Leaf: Based on Renesas M32C, running firmware labeled as 'filo'
+ * - UsbcanII: Based on Renesas M16C, running firmware labeled as 'helios'
+ */
+enum kvaser_usb_family {
+ KVASER_LEAF,
+ KVASER_USBCAN,
+};
+
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
+
#define MAX_TX_URBS 16
#define MAX_RX_URBS 4
#define START_TIMEOUT 1000 /* msecs */
@@ -29,9 +43,12 @@
#define USB_RECV_TIMEOUT 1000 /* msecs */
#define RX_BUFFER_SIZE 3072
#define CAN_USB_CLOCK 8000000
-#define MAX_NET_DEVICES 3
+#define LEAF_MAX_NET_DEVICES 3
+#define USBCAN_MAX_NET_DEVICES 2
+#define MAX_NET_DEVICES MAX(LEAF_MAX_NET_DEVICES, \
+ USBCAN_MAX_NET_DEVICES)
-/* Kvaser USB devices */
+/* Leaf USB devices */
#define KVASER_VENDOR_ID 0x0bfd
#define USB_LEAF_DEVEL_PRODUCT_ID 10
#define USB_LEAF_LITE_PRODUCT_ID 11
@@ -55,6 +72,16 @@
#define USB_CAN_R_PRODUCT_ID 39
#define USB_LEAF_LITE_V2_PRODUCT_ID 288
#define USB_MINI_PCIE_HS_PRODUCT_ID 289
+#define LEAF_PRODUCT_ID(id) (id >= USB_LEAF_DEVEL_PRODUCT_ID && \
+ id <= USB_MINI_PCIE_HS_PRODUCT_ID)
+
+/* USBCANII devices */
+#define USB_USBCAN_REVB_PRODUCT_ID 2
+#define USB_VCI2_PRODUCT_ID 3
+#define USB_USBCAN2_PRODUCT_ID 4
+#define USB_MEMORATOR_PRODUCT_ID 5
+#define USBCAN_PRODUCT_ID(id) (id >= USB_USBCAN_REVB_PRODUCT_ID && \
+ id <= USB_MEMORATOR_PRODUCT_ID)
/* USB devices features */
#define KVASER_HAS_SILENT_MODE BIT(0)
@@ -73,7 +100,7 @@
#define MSG_FLAG_TX_ACK BIT(6)
#define MSG_FLAG_TX_REQUEST BIT(7)
-/* Can states */
+/* Can states (M16C CxSTRH register) */
#define M16C_STATE_BUS_RESET BIT(0)
#define M16C_STATE_BUS_ERROR BIT(4)
#define M16C_STATE_BUS_PASSIVE BIT(5)
@@ -98,7 +125,13 @@
#define CMD_START_CHIP_REPLY 27
#define CMD_STOP_CHIP 28
#define CMD_STOP_CHIP_REPLY 29
-#define CMD_GET_CARD_INFO2 32
+#define CMD_READ_CLOCK 30
+#define CMD_READ_CLOCK_REPLY 31
+
+#define LEAF_CMD_GET_CARD_INFO2 32
+#define USBCAN_CMD_RESET_CLOCK 32
+#define USBCAN_CMD_CLOCK_OVERFLOW_EVENT 33
+
#define CMD_GET_CARD_INFO 34
#define CMD_GET_CARD_INFO_REPLY 35
#define CMD_GET_SOFTWARE_INFO 38
@@ -108,8 +141,9 @@
#define CMD_RESET_ERROR_COUNTER 49
#define CMD_TX_ACKNOWLEDGE 50
#define CMD_CAN_ERROR_EVENT 51
-#define CMD_USB_THROTTLE 77
-#define CMD_LOG_MESSAGE 106
+
+#define LEAF_CMD_USB_THROTTLE 77
+#define LEAF_CMD_LOG_MESSAGE 106
/* error factors */
#define M16C_EF_ACKE BIT(0)
@@ -121,6 +155,13 @@
#define M16C_EF_RCVE BIT(6)
#define M16C_EF_TRE BIT(7)
+/* Only Leaf-based devices can report M16C error factors,
+ * thus define our own error status flags for USBCAN */
+#define USBCAN_ERROR_STATE_NONE 0
+#define USBCAN_ERROR_STATE_TX_ERROR BIT(0)
+#define USBCAN_ERROR_STATE_RX_ERROR BIT(1)
+#define USBCAN_ERROR_STATE_BUSERROR BIT(2)
+
/* bittiming parameters */
#define KVASER_USB_TSEG1_MIN 1
#define KVASER_USB_TSEG1_MAX 16
@@ -137,7 +178,7 @@
#define KVASER_CTRL_MODE_SELFRECEPTION 3
#define KVASER_CTRL_MODE_OFF 4
-/* log message */
+/* Extended CAN identifier flag */
#define KVASER_EXTENDED_FRAME BIT(31)
struct kvaser_msg_simple {
@@ -148,30 +189,55 @@ struct kvaser_msg_simple {
struct kvaser_msg_cardinfo {
u8 tid;
u8 nchannels;
- __le32 serial_number;
- __le32 padding;
+ union {
+ struct {
+ __le32 serial_number;
+ __le32 padding;
+ } __packed leaf0;
+ struct {
+ __le32 serial_number_low;
+ __le32 serial_number_high;
+ } __packed usbcan0;
+ } __packed;
__le32 clock_resolution;
__le32 mfgdate;
u8 ean[8];
u8 hw_revision;
- u8 usb_hs_mode;
- __le16 padding2;
+ union {
+ struct {
+ u8 usb_hs_mode;
+ } __packed leaf1;
+ struct {
+ u8 padding;
+ } __packed usbcan1;
+ } __packed;
+ __le16 padding;
} __packed;
struct kvaser_msg_cardinfo2 {
u8 tid;
- u8 channel;
+ u8 reserved;
u8 pcb_id[24];
__le32 oem_unlock_code;
} __packed;
-struct kvaser_msg_softinfo {
+struct leaf_msg_softinfo {
u8 tid;
- u8 channel;
+ u8 padding0;
__le32 sw_options;
__le32 fw_version;
__le16 max_outstanding_tx;
- __le16 padding[9];
+ __le16 padding1[9];
+} __packed;
+
+struct usbcan_msg_softinfo {
+ u8 tid;
+ u8 fw_name[5];
+ __le16 max_outstanding_tx;
+ u8 padding[6];
+ __le32 fw_version;
+ __le16 checksum;
+ __le16 sw_options;
} __packed;
struct kvaser_msg_busparams {
@@ -188,36 +254,86 @@ struct kvaser_msg_tx_can {
u8 channel;
u8 tid;
u8 msg[14];
- u8 padding;
- u8 flags;
+ union {
+ struct {
+ u8 padding;
+ u8 flags;
+ } __packed leaf;
+ struct {
+ u8 flags;
+ u8 padding;
+ } __packed usbcan;
+ } __packed;
+} __packed;
+
+struct kvaser_msg_rx_can_header {
+ u8 channel;
+ u8 flag;
} __packed;
-struct kvaser_msg_rx_can {
+struct leaf_msg_rx_can {
u8 channel;
u8 flag;
+
__le16 time[3];
u8 msg[14];
} __packed;
-struct kvaser_msg_chip_state_event {
+struct usbcan_msg_rx_can {
+ u8 channel;
+ u8 flag;
+
+ u8 msg[14];
+ __le16 time;
+} __packed;
+
+struct leaf_msg_chip_state_event {
u8 tid;
u8 channel;
+
__le16 time[3];
u8 tx_errors_count;
u8 rx_errors_count;
+
u8 status;
u8 padding[3];
} __packed;
-struct kvaser_msg_tx_acknowledge {
+struct usbcan_msg_chip_state_event {
+ u8 tid;
+ u8 channel;
+
+ u8 tx_errors_count;
+ u8 rx_errors_count;
+ __le16 time;
+
+ u8 status;
+ u8 padding[3];
+} __packed;
+
+struct kvaser_msg_tx_acknowledge_header {
+ u8 channel;
+ u8 tid;
+};
+
+struct leaf_msg_tx_acknowledge {
u8 channel;
u8 tid;
+
__le16 time[3];
u8 flags;
u8 time_offset;
} __packed;
-struct kvaser_msg_error_event {
+struct usbcan_msg_tx_acknowledge {
+ u8 channel;
+ u8 tid;
+
+ __le16 time;
+ __le16 padding;
+} __packed;
+
+struct leaf_msg_error_event {
u8 tid;
u8 flags;
__le16 time[3];
@@ -229,6 +345,18 @@ struct kvaser_msg_error_event {
u8 error_factor;
} __packed;
+struct usbcan_msg_error_event {
+ u8 tid;
+ u8 padding;
+ u8 tx_errors_count_ch0;
+ u8 rx_errors_count_ch0;
+ u8 tx_errors_count_ch1;
+ u8 rx_errors_count_ch1;
+ u8 status_ch0;
+ u8 status_ch1;
+ __le16 time;
+} __packed;
+
struct kvaser_msg_ctrl_mode {
u8 tid;
u8 channel;
@@ -243,7 +371,7 @@ struct kvaser_msg_flush_queue {
u8 padding[3];
} __packed;
-struct kvaser_msg_log_message {
+struct leaf_msg_log_message {
u8 channel;
u8 flags;
__le16 time[3];
@@ -260,19 +388,49 @@ struct kvaser_msg {
struct kvaser_msg_simple simple;
struct kvaser_msg_cardinfo cardinfo;
struct kvaser_msg_cardinfo2 cardinfo2;
- struct kvaser_msg_softinfo softinfo;
struct kvaser_msg_busparams busparams;
+
+ struct kvaser_msg_rx_can_header rx_can_header;
+ struct kvaser_msg_tx_acknowledge_header tx_acknowledge_header;
+
+ union {
+ struct leaf_msg_softinfo softinfo;
+ struct leaf_msg_rx_can rx_can;
+ struct leaf_msg_chip_state_event chip_state_event;
+ struct leaf_msg_tx_acknowledge tx_acknowledge;
+ struct leaf_msg_error_event error_event;
+ struct leaf_msg_log_message log_message;
+ } __packed leaf;
+
+ union {
+ struct usbcan_msg_softinfo softinfo;
+ struct usbcan_msg_rx_can rx_can;
+ struct usbcan_msg_chip_state_event chip_state_event;
+ struct usbcan_msg_tx_acknowledge tx_acknowledge;
+ struct usbcan_msg_error_event error_event;
+ } __packed usbcan;
+
struct kvaser_msg_tx_can tx_can;
- struct kvaser_msg_rx_can rx_can;
- struct kvaser_msg_chip_state_event chip_state_event;
- struct kvaser_msg_tx_acknowledge tx_acknowledge;
- struct kvaser_msg_error_event error_event;
struct kvaser_msg_ctrl_mode ctrl_mode;
struct kvaser_msg_flush_queue flush_queue;
- struct kvaser_msg_log_message log_message;
} u;
} __packed;
+/* Leaf/USBCAN-agnostic summary of an error event.
+ * No M16C error factors for USBCAN-based devices. */
+struct kvaser_error_summary {
+ u8 channel, status, txerr, rxerr;
+ union {
+ struct {
+ u8 error_factor;
+ } leaf;
+ struct {
+ u8 other_ch_status;
+ u8 error_state;
+ } usbcan;
+ };
+};
+
struct kvaser_usb_tx_urb_context {
struct kvaser_usb_net_priv *priv;
u32 echo_index;
@@ -288,6 +446,8 @@ struct kvaser_usb {
u32 fw_version;
unsigned int nchannels;
+ enum kvaser_usb_family family;
+ unsigned int max_channels;
bool rxinitdone;
void *rxbuf[MAX_RX_URBS];
@@ -311,6 +471,7 @@ struct kvaser_usb_net_priv {
};
static const struct usb_device_id kvaser_usb_table[] = {
+ /* Leaf family IDs */
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_DEVEL_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_PRO_PRODUCT_ID),
@@ -360,6 +521,17 @@ static const struct usb_device_id kvaser_usb_table[] = {
.driver_info = KVASER_HAS_TXRX_ERRORS },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_LEAF_LITE_V2_PRODUCT_ID) },
{ USB_DEVICE(KVASER_VENDOR_ID, USB_MINI_PCIE_HS_PRODUCT_ID) },
+
+ /* USBCANII family IDs */
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN2_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_USBCAN_REVB_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_MEMORATOR_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+ { USB_DEVICE(KVASER_VENDOR_ID, USB_VCI2_PRODUCT_ID),
+ .driver_info = KVASER_HAS_TXRX_ERRORS },
+
{ }
};
MODULE_DEVICE_TABLE(usb, kvaser_usb_table);
@@ -463,7 +635,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
if (err)
return err;
- dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
+ switch (dev->family) {
+ case KVASER_LEAF:
+ dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+ break;
+ case KVASER_USBCAN:
+ dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
+ return -EINVAL;
+ }
return 0;
}
@@ -482,7 +665,7 @@ static int kvaser_usb_get_card_info(struct kvaser_usb *dev)
return err;
dev->nchannels = msg.u.cardinfo.nchannels;
- if (dev->nchannels > MAX_NET_DEVICES)
+ if (dev->nchannels > dev->max_channels)
return -EINVAL;
return 0;
@@ -496,8 +679,10 @@ static void kvaser_usb_tx_acknowledge(const struct kvaser_usb *dev,
struct kvaser_usb_net_priv *priv;
struct sk_buff *skb;
struct can_frame *cf;
- u8 channel = msg->u.tx_acknowledge.channel;
- u8 tid = msg->u.tx_acknowledge.tid;
+ u8 channel, tid;
+
+ channel = msg->u.tx_acknowledge_header.channel;
+ tid = msg->u.tx_acknowledge_header.tid;
if (channel >= dev->nchannels) {
dev_err(dev->udev->dev.parent,
@@ -615,37 +800,83 @@ static void kvaser_usb_unlink_tx_urbs(struct kvaser_usb_net_priv *priv)
priv->tx_contexts[i].echo_index = MAX_TX_URBS;
}
-static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
- const struct kvaser_msg *msg)
+static void kvaser_report_error_event(const struct kvaser_usb *dev,
+ struct kvaser_error_summary *es);
+
+/*
+ * Report error to userspace iff the controller's errors counter has
+ * increased, or we're the only channel seeing the bus error state.
+ *
+ * As reported by USBCAN sheets, "the CAN controller has difficulties
+ * to tell whether an error frame arrived on channel 1 or on channel 2."
+ * Thus, error counters are compared with their earlier values to
+ * determine which channel was responsible for the error event.
+ */
+static void usbcan_report_error_if_applicable(const struct kvaser_usb *dev,
+ struct kvaser_error_summary *es)
{
- struct can_frame *cf;
- struct sk_buff *skb;
- struct net_device_stats *stats;
struct kvaser_usb_net_priv *priv;
- unsigned int new_state;
- u8 channel, status, txerr, rxerr, error_factor;
+ int old_tx_err_count, old_rx_err_count, channel, report_error;
+
+ channel = es->channel;
+ if (channel >= dev->nchannels) {
+ dev_err(dev->udev->dev.parent,
+ "Invalid channel number (%d)\n", channel);
+ return;
+ }
+
+ priv = dev->nets[channel];
+ old_tx_err_count = priv->bec.txerr;
+ old_rx_err_count = priv->bec.rxerr;
+
+ report_error = 0;
+ if (es->txerr > old_tx_err_count) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_TX_ERROR;
+ report_error = 1;
+ }
+ if (es->rxerr > old_rx_err_count) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_RX_ERROR;
+ report_error = 1;
+ }
+ if ((es->status & M16C_STATE_BUS_ERROR) &&
+ !(es->usbcan.other_ch_status & M16C_STATE_BUS_ERROR)) {
+ es->usbcan.error_state |= USBCAN_ERROR_STATE_BUSERROR;
+ report_error = 1;
+ }
+
+ if (report_error)
+ kvaser_report_error_event(dev, es);
+}
+
+/*
+ * Extract error summary from a Leaf-based device error message
+ */
+static void leaf_extract_error_from_msg(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ struct kvaser_error_summary es = { 0, };
switch (msg->id) {
case CMD_CAN_ERROR_EVENT:
- channel = msg->u.error_event.channel;
- status = msg->u.error_event.status;
- txerr = msg->u.error_event.tx_errors_count;
- rxerr = msg->u.error_event.rx_errors_count;
- error_factor = msg->u.error_event.error_factor;
+ es.channel = msg->u.leaf.error_event.channel;
+ es.status = msg->u.leaf.error_event.status;
+ es.txerr = msg->u.leaf.error_event.tx_errors_count;
+ es.rxerr = msg->u.leaf.error_event.rx_errors_count;
+ es.leaf.error_factor = msg->u.leaf.error_event.error_factor;
break;
- case CMD_LOG_MESSAGE:
- channel = msg->u.log_message.channel;
- status = msg->u.log_message.data[0];
- txerr = msg->u.log_message.data[2];
- rxerr = msg->u.log_message.data[3];
- error_factor = msg->u.log_message.data[1];
+ case LEAF_CMD_LOG_MESSAGE:
+ es.channel = msg->u.leaf.log_message.channel;
+ es.status = msg->u.leaf.log_message.data[0];
+ es.txerr = msg->u.leaf.log_message.data[2];
+ es.rxerr = msg->u.leaf.log_message.data[3];
+ es.leaf.error_factor = msg->u.leaf.log_message.data[1];
break;
case CMD_CHIP_STATE_EVENT:
- channel = msg->u.chip_state_event.channel;
- status = msg->u.chip_state_event.status;
- txerr = msg->u.chip_state_event.tx_errors_count;
- rxerr = msg->u.chip_state_event.rx_errors_count;
- error_factor = 0;
+ es.channel = msg->u.leaf.chip_state_event.channel;
+ es.status = msg->u.leaf.chip_state_event.status;
+ es.txerr = msg->u.leaf.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.leaf.chip_state_event.rx_errors_count;
+ es.leaf.error_factor = 0;
break;
default:
dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
@@ -653,16 +884,92 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
return;
}
- if (channel >= dev->nchannels) {
+ kvaser_report_error_event(dev, &es);
+}
+
+/*
+ * Extract summary from a USBCANII-based device error message.
+ */
+static void usbcan_extract_error_from_msg(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ struct kvaser_error_summary es = { 0, };
+
+ switch (msg->id) {
+
+ /* Sometimes errors are sent as unsolicited chip state events */
+ case CMD_CHIP_STATE_EVENT:
+ es.channel = msg->u.usbcan.chip_state_event.channel;
+ es.status = msg->u.usbcan.chip_state_event.status;
+ es.txerr = msg->u.usbcan.chip_state_event.tx_errors_count;
+ es.rxerr = msg->u.usbcan.chip_state_event.rx_errors_count;
+ usbcan_report_error_if_applicable(dev, &es);
+ break;
+
+ case CMD_CAN_ERROR_EVENT:
+ es.channel = 0;
+ es.status = msg->u.usbcan.error_event.status_ch0;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch0;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch0;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch1;
+ usbcan_report_error_if_applicable(dev, &es);
+
+ /* For error events, the USBCAN firmware does not support
+ * more than 2 channels: ch0, and ch1. */
+ if (dev->nchannels > 1) {
+ es.channel = 1;
+ es.status = msg->u.usbcan.error_event.status_ch1;
+ es.txerr = msg->u.usbcan.error_event.tx_errors_count_ch1;
+ es.rxerr = msg->u.usbcan.error_event.rx_errors_count_ch1;
+ es.usbcan.other_ch_status =
+ msg->u.usbcan.error_event.status_ch0;
+ usbcan_report_error_if_applicable(dev, &es);
+ }
+ break;
+
+ default:
+ dev_err(dev->udev->dev.parent, "Invalid msg id (%d)\n",
+ msg->id);
+ }
+}
+
+static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
+ const struct kvaser_msg *msg)
+{
+ switch (dev->family) {
+ case KVASER_LEAF:
+ leaf_extract_error_from_msg(dev, msg);
+ break;
+ case KVASER_USBCAN:
+ usbcan_extract_error_from_msg(dev, msg);
+ break;
+ default:
dev_err(dev->udev->dev.parent,
- "Invalid channel number (%d)\n", channel);
+ "Invalid device family (%d)\n", dev->family);
return;
}
+}
- priv = dev->nets[channel];
+static void kvaser_report_error_event(const struct kvaser_usb *dev,
+ struct kvaser_error_summary *es)
+{
+ struct can_frame *cf;
+ struct sk_buff *skb;
+ struct net_device_stats *stats;
+ struct kvaser_usb_net_priv *priv;
+ unsigned int new_state;
+
+ if (es->channel >= dev->nchannels) {
+ dev_err(dev->udev->dev.parent,
+ "Invalid channel number (%d)\n", es->channel);
+ return;
+ }
+
+ priv = dev->nets[es->channel];
stats = &priv->netdev->stats;
- if (status & M16C_STATE_BUS_RESET) {
+ if (es->status & M16C_STATE_BUS_RESET) {
kvaser_usb_unlink_tx_urbs(priv);
return;
}
@@ -675,9 +982,9 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
new_state = priv->can.state;
- netdev_dbg(priv->netdev, "Error status: 0x%02x\n", status);
+ netdev_dbg(priv->netdev, "Error status: 0x%02x\n", es->status);
- if (status & M16C_STATE_BUS_OFF) {
+ if (es->status & M16C_STATE_BUS_OFF) {
cf->can_id |= CAN_ERR_BUSOFF;
priv->can.can_stats.bus_off++;
@@ -687,12 +994,12 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
netif_carrier_off(priv->netdev);
new_state = CAN_STATE_BUS_OFF;
- } else if (status & M16C_STATE_BUS_PASSIVE) {
+ } else if (es->status & M16C_STATE_BUS_PASSIVE) {
if (priv->can.state != CAN_STATE_ERROR_PASSIVE) {
cf->can_id |= CAN_ERR_CRTL;
- if (txerr || rxerr)
- cf->data[1] = (txerr > rxerr)
+ if (es->txerr || es->rxerr)
+ cf->data[1] = (es->txerr > es->rxerr)
? CAN_ERR_CRTL_TX_PASSIVE
: CAN_ERR_CRTL_RX_PASSIVE;
else
@@ -703,13 +1010,11 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
}
new_state = CAN_STATE_ERROR_PASSIVE;
- }
-
- if (status == M16C_STATE_BUS_ERROR) {
+ } else if (es->status & M16C_STATE_BUS_ERROR) {
if ((priv->can.state < CAN_STATE_ERROR_WARNING) &&
- ((txerr >= 96) || (rxerr >= 96))) {
+ ((es->txerr >= 96) || (es->rxerr >= 96))) {
cf->can_id |= CAN_ERR_CRTL;
- cf->data[1] = (txerr > rxerr)
+ cf->data[1] = (es->txerr > es->rxerr)
? CAN_ERR_CRTL_TX_WARNING
: CAN_ERR_CRTL_RX_WARNING;
@@ -723,7 +1028,7 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
}
}
- if (!status) {
+ if (!es->status) {
cf->can_id |= CAN_ERR_PROT;
cf->data[2] = CAN_ERR_PROT_ACTIVE;
@@ -739,34 +1044,52 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
priv->can.can_stats.restarts++;
}
- if (error_factor) {
- priv->can.can_stats.bus_error++;
- stats->rx_errors++;
-
- cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
-
- if (error_factor & M16C_EF_ACKE)
- cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
- if (error_factor & M16C_EF_CRCE)
- cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
- CAN_ERR_PROT_LOC_CRC_DEL);
- if (error_factor & M16C_EF_FORME)
- cf->data[2] |= CAN_ERR_PROT_FORM;
- if (error_factor & M16C_EF_STFE)
- cf->data[2] |= CAN_ERR_PROT_STUFF;
- if (error_factor & M16C_EF_BITE0)
- cf->data[2] |= CAN_ERR_PROT_BIT0;
- if (error_factor & M16C_EF_BITE1)
- cf->data[2] |= CAN_ERR_PROT_BIT1;
- if (error_factor & M16C_EF_TRE)
- cf->data[2] |= CAN_ERR_PROT_TX;
+ switch (dev->family) {
+ case KVASER_LEAF:
+ if (es->leaf.error_factor) {
+ priv->can.can_stats.bus_error++;
+ stats->rx_errors++;
+
+ cf->can_id |= CAN_ERR_BUSERROR | CAN_ERR_PROT;
+
+ if (es->leaf.error_factor & M16C_EF_ACKE)
+ cf->data[3] |= (CAN_ERR_PROT_LOC_ACK);
+ if (es->leaf.error_factor & M16C_EF_CRCE)
+ cf->data[3] |= (CAN_ERR_PROT_LOC_CRC_SEQ |
+ CAN_ERR_PROT_LOC_CRC_DEL);
+ if (es->leaf.error_factor & M16C_EF_FORME)
+ cf->data[2] |= CAN_ERR_PROT_FORM;
+ if (es->leaf.error_factor & M16C_EF_STFE)
+ cf->data[2] |= CAN_ERR_PROT_STUFF;
+ if (es->leaf.error_factor & M16C_EF_BITE0)
+ cf->data[2] |= CAN_ERR_PROT_BIT0;
+ if (es->leaf.error_factor & M16C_EF_BITE1)
+ cf->data[2] |= CAN_ERR_PROT_BIT1;
+ if (es->leaf.error_factor & M16C_EF_TRE)
+ cf->data[2] |= CAN_ERR_PROT_TX;
+ }
+ break;
+ case KVASER_USBCAN:
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_TX_ERROR)
+ stats->tx_errors++;
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_RX_ERROR)
+ stats->rx_errors++;
+ if (es->usbcan.error_state & USBCAN_ERROR_STATE_BUSERROR) {
+ priv->can.can_stats.bus_error++;
+ cf->can_id |= CAN_ERR_BUSERROR;
+ }
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
+ goto err;
}
- cf->data[6] = txerr;
- cf->data[7] = rxerr;
+ cf->data[6] = es->txerr;
+ cf->data[7] = es->rxerr;
- priv->bec.txerr = txerr;
- priv->bec.rxerr = rxerr;
+ priv->bec.txerr = es->txerr;
+ priv->bec.rxerr = es->rxerr;
priv->can.state = new_state;
@@ -774,6 +1097,11 @@ static void kvaser_usb_rx_error(const struct kvaser_usb *dev,
stats->rx_packets++;
stats->rx_bytes += cf->can_dlc;
+
+ return;
+
+err:
+ dev_kfree_skb(skb);
}
static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
@@ -783,16 +1111,16 @@ static void kvaser_usb_rx_can_err(const struct kvaser_usb_net_priv *priv,
struct sk_buff *skb;
struct net_device_stats *stats = &priv->netdev->stats;
- if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
+ if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
MSG_FLAG_NERR)) {
netdev_err(priv->netdev, "Unknow error (flags: 0x%02x)\n",
- msg->u.rx_can.flag);
+ msg->u.rx_can_header.flag);
stats->rx_errors++;
return;
}
- if (msg->u.rx_can.flag & MSG_FLAG_OVERRUN) {
+ if (msg->u.rx_can_header.flag & MSG_FLAG_OVERRUN) {
skb = alloc_can_err_skb(priv->netdev, &cf);
if (!skb) {
stats->rx_dropped++;
@@ -819,7 +1147,8 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
struct can_frame *cf;
struct sk_buff *skb;
struct net_device_stats *stats;
- u8 channel = msg->u.rx_can.channel;
+ u8 channel = msg->u.rx_can_header.channel;
+ const u8 *rx_msg;
if (channel >= dev->nchannels) {
dev_err(dev->udev->dev.parent,
@@ -830,19 +1159,32 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
priv = dev->nets[channel];
stats = &priv->netdev->stats;
- if ((msg->u.rx_can.flag & MSG_FLAG_ERROR_FRAME) &&
- (msg->id == CMD_LOG_MESSAGE)) {
+ if ((msg->u.rx_can_header.flag & MSG_FLAG_ERROR_FRAME) &&
+ (dev->family == KVASER_LEAF && msg->id == LEAF_CMD_LOG_MESSAGE)) {
kvaser_usb_rx_error(dev, msg);
return;
- } else if (msg->u.rx_can.flag & (MSG_FLAG_ERROR_FRAME |
- MSG_FLAG_NERR |
- MSG_FLAG_OVERRUN)) {
+ } else if (msg->u.rx_can_header.flag & (MSG_FLAG_ERROR_FRAME |
+ MSG_FLAG_NERR |
+ MSG_FLAG_OVERRUN)) {
kvaser_usb_rx_can_err(priv, msg);
return;
- } else if (msg->u.rx_can.flag & ~MSG_FLAG_REMOTE_FRAME) {
+ } else if (msg->u.rx_can_header.flag & ~MSG_FLAG_REMOTE_FRAME) {
netdev_warn(priv->netdev,
"Unhandled frame (flags: 0x%02x)",
- msg->u.rx_can.flag);
+ msg->u.rx_can_header.flag);
+ return;
+ }
+
+ switch (dev->family) {
+ case KVASER_LEAF:
+ rx_msg = msg->u.leaf.rx_can.msg;
+ break;
+ case KVASER_USBCAN:
+ rx_msg = msg->u.usbcan.rx_can.msg;
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
return;
}
@@ -852,38 +1194,37 @@ static void kvaser_usb_rx_can_msg(const struct kvaser_usb *dev,
return;
}
- if (msg->id == CMD_LOG_MESSAGE) {
- cf->can_id = le32_to_cpu(msg->u.log_message.id);
+ if (dev->family == KVASER_LEAF && msg->id == LEAF_CMD_LOG_MESSAGE) {
+ cf->can_id = le32_to_cpu(msg->u.leaf.log_message.id);
if (cf->can_id & KVASER_EXTENDED_FRAME)
cf->can_id &= CAN_EFF_MASK | CAN_EFF_FLAG;
else
cf->can_id &= CAN_SFF_MASK;
- cf->can_dlc = get_can_dlc(msg->u.log_message.dlc);
+ cf->can_dlc = get_can_dlc(msg->u.leaf.log_message.dlc);
- if (msg->u.log_message.flags & MSG_FLAG_REMOTE_FRAME)
+ if (msg->u.leaf.log_message.flags & MSG_FLAG_REMOTE_FRAME)
cf->can_id |= CAN_RTR_FLAG;
else
- memcpy(cf->data, &msg->u.log_message.data,
+ memcpy(cf->data, &msg->u.leaf.log_message.data,
cf->can_dlc);
} else {
- cf->can_id = ((msg->u.rx_can.msg[0] & 0x1f) << 6) |
- (msg->u.rx_can.msg[1] & 0x3f);
+ cf->can_id = ((rx_msg[0] & 0x1f) << 6) | (rx_msg[1] & 0x3f);
if (msg->id == CMD_RX_EXT_MESSAGE) {
cf->can_id <<= 18;
- cf->can_id |= ((msg->u.rx_can.msg[2] & 0x0f) << 14) |
- ((msg->u.rx_can.msg[3] & 0xff) << 6) |
- (msg->u.rx_can.msg[4] & 0x3f);
+ cf->can_id |= ((rx_msg[2] & 0x0f) << 14) |
+ ((rx_msg[3] & 0xff) << 6) |
+ (rx_msg[4] & 0x3f);
cf->can_id |= CAN_EFF_FLAG;
}
- cf->can_dlc = get_can_dlc(msg->u.rx_can.msg[5]);
+ cf->can_dlc = get_can_dlc(rx_msg[5]);
- if (msg->u.rx_can.flag & MSG_FLAG_REMOTE_FRAME)
+ if (msg->u.rx_can_header.flag & MSG_FLAG_REMOTE_FRAME)
cf->can_id |= CAN_RTR_FLAG;
else
- memcpy(cf->data, &msg->u.rx_can.msg[6],
+ memcpy(cf->data, &rx_msg[6],
cf->can_dlc);
}
@@ -947,7 +1288,12 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
case CMD_RX_STD_MESSAGE:
case CMD_RX_EXT_MESSAGE:
- case CMD_LOG_MESSAGE:
+ kvaser_usb_rx_can_msg(dev, msg);
+ break;
+
+ case LEAF_CMD_LOG_MESSAGE:
+ if (dev->family != KVASER_LEAF)
+ goto warn;
kvaser_usb_rx_can_msg(dev, msg);
break;
@@ -960,8 +1306,14 @@ static void kvaser_usb_handle_message(const struct kvaser_usb *dev,
kvaser_usb_tx_acknowledge(dev, msg);
break;
+ /* Ignored messages */
+ case USBCAN_CMD_CLOCK_OVERFLOW_EVENT:
+ if (dev->family != KVASER_USBCAN)
+ goto warn;
+ break;
+
default:
- dev_warn(dev->udev->dev.parent,
+warn: dev_warn(dev->udev->dev.parent,
"Unhandled message (%d)\n", msg->id);
break;
}
@@ -1181,7 +1533,7 @@ static void kvaser_usb_unlink_all_urbs(struct kvaser_usb *dev)
dev->rxbuf[i],
dev->rxbuf_dma[i]);
- for (i = 0; i < MAX_NET_DEVICES; i++) {
+ for (i = 0; i < dev->nchannels; i++) {
struct kvaser_usb_net_priv *priv = dev->nets[i];
if (priv)
@@ -1289,6 +1641,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
struct kvaser_msg *msg;
int i, err;
int ret = NETDEV_TX_OK;
+ uint8_t *msg_tx_can_flags;
if (can_dropped_invalid_skb(netdev, skb))
return NETDEV_TX_OK;
@@ -1310,9 +1663,23 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
msg = buf;
msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
- msg->u.tx_can.flags = 0;
msg->u.tx_can.channel = priv->channel;
+ switch (dev->family) {
+ case KVASER_LEAF:
+ msg_tx_can_flags = &msg->u.tx_can.leaf.flags;
+ break;
+ case KVASER_USBCAN:
+ msg_tx_can_flags = &msg->u.tx_can.usbcan.flags;
+ break;
+ default:
+ dev_err(dev->udev->dev.parent,
+ "Invalid device family (%d)\n", dev->family);
+ goto releasebuf;
+ }
+
+ *msg_tx_can_flags = 0;
+
if (cf->can_id & CAN_EFF_FLAG) {
msg->id = CMD_TX_EXT_MESSAGE;
msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
@@ -1330,7 +1697,7 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
if (cf->can_id & CAN_RTR_FLAG)
- msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
+ *msg_tx_can_flags |= MSG_FLAG_REMOTE_FRAME;
for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
@@ -1601,6 +1968,18 @@ static int kvaser_usb_probe(struct usb_interface *intf,
if (!dev)
return -ENOMEM;
+ if (LEAF_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_LEAF;
+ dev->max_channels = LEAF_MAX_NET_DEVICES;
+ } else if (USBCAN_PRODUCT_ID(id->idProduct)) {
+ dev->family = KVASER_USBCAN;
+ dev->max_channels = USBCAN_MAX_NET_DEVICES;
+ } else {
+ dev_err(&intf->dev, "Product ID (%d) does not belong to any "
+ "known Kvaser USB family", id->idProduct);
+ return -ENODEV;
+ }
+
err = kvaser_usb_get_endpoints(intf, &dev->bulk_in, &dev->bulk_out);
if (err) {
dev_err(&intf->dev, "Cannot get usb endpoint(s)");
^ permalink raw reply related
* [PATCH v2 3/4] can: kvaser_usb: Don't send a RESET_CHIP for non-existing channels
From: Ahmed S. Darwish @ 2014-12-25 0:02 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Greg KH, Linux-CAN, netdev, LKML
In-Reply-To: <20141224235954.GA24302@vivalin-002>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
"Someone reported me that recent leaf firmwares go in trouble when
you send a command for a channel that does not exist. Instead ...
you can move the reset command to kvaser_usb_init_one() function."
Suggested-by: Olivier Sobrie <olivier@sobrie.be>
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 598e251..2791501 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1505,6 +1505,10 @@ static int kvaser_usb_init_one(struct usb_interface *intf,
struct kvaser_usb_net_priv *priv;
int i, err;
+ err = kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, channel);
+ if (err)
+ return err;
+
netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
if (!netdev) {
dev_err(&intf->dev, "Cannot alloc candev\n");
@@ -1609,9 +1613,6 @@ static int kvaser_usb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, dev);
- for (i = 0; i < MAX_NET_DEVICES; i++)
- kvaser_usb_send_simple_msg(dev, CMD_RESET_CHIP, i);
-
err = kvaser_usb_get_software_info(dev);
if (err) {
dev_err(&intf->dev,
^ permalink raw reply related
* [PATCH v2 2/4] can: kvaser_usb: Reset all URB tx contexts upon channel close
From: Ahmed S. Darwish @ 2014-12-24 23:59 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, Greg KH,
Linux-stable, LKML
In-Reply-To: <20141224235644.GA3778@vivalin-002>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in very high frequency (*), closing the CAN channel while
all the transmissions are on (#), opening the device again (@),
then sending a small number of packets would make the driver
enter an almost infinite loop of:
[....]
[15959.853988] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853990] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853991] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853993] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853994] kvaser_usb 4-3:1.0 can0: cannot find free context
[15959.853995] kvaser_usb 4-3:1.0 can0: cannot find free context
[....]
_dragging the whole system down_ in the process due to the
excessive logging output.
Initially, this has caused random panics in the kernel due to a
buggy error recovery path. That got fixed in an earlier commit.(%)
This patch aims at solving the root cause. -->
16 tx URBs and contexts are allocated per CAN channel per USB
device. Such URBs are protected by:
a) A simple atomic counter, up to a value of MAX_TX_URBS (16)
b) A flag in each URB context, stating if it's free
c) The fact that ndo_start_xmit calls are themselves protected
by the networking layers higher above
After grabbing one of the tx URBs, if the driver noticed that all
of them are now taken, it stops the netif transmission queue.
Such queue is worken up again only if an acknowedgment was received
from the firmware on one of our earlier-sent frames.
Meanwhile, upon channel close (#), the driver sends a CMD_STOP_CHIP
to the firmware, effectively closing all further communication. In
the high traffic case, the atomic counter remains at MAX_TX_URBS,
and all the URB contexts remain marked as active. While opening
the channel again (@), it cannot send any further frames since no
more free tx URB contexts are available.
Reset all tx URB contexts upon CAN channel close.
(*) 50 parallel instances of `cangen0 -g 0 -ix`
(#) `ifconfig can0 down`
(@) `ifconfig can0 up`
(%) "can: kvaser_usb: Don't free packets when tight on URBs"
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 3 +++
1 file changed, 3 insertions(+)
(Marc, Greg, this also should be added to -stable?)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 6479a2b..598e251 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1246,6 +1246,9 @@ static int kvaser_usb_close(struct net_device *netdev)
if (err)
netdev_warn(netdev, "Cannot stop device, error %d\n", err);
+ /* reset tx contexts */
+ kvaser_usb_unlink_tx_urbs(priv);
+
priv->can.state = CAN_STATE_STOPPED;
close_candev(priv->netdev);
^ permalink raw reply related
* [PATCH v2 1/4] can: kvaser_usb: Don't free packets when tight on URBs
From: Ahmed S. Darwish @ 2014-12-24 23:56 UTC (permalink / raw)
To: Olivier Sobrie, Oliver Hartkopp, Wolfgang Grandegger,
Marc Kleine-Budde
Cc: David S. Miller, Paul Gortmaker, Linux-CAN, netdev, Greg KH,
Linux-stable, LKML
In-Reply-To: <20141223154654.GB6460@vivalin-002>
From: Ahmed S. Darwish <ahmed.darwish@valeo.com>
Flooding the Kvaser CAN to USB dongle with multiple reads and
writes in high frequency caused seemingly-random panics in the
kernel.
On further inspection, it seems the driver erroneously freed the
to-be-transmitted packet upon getting tight on URBs and returning
NETDEV_TX_BUSY, leading to invalid memory writes and double frees
at a later point in time.
Note:
Finding no more URBs/transmit-contexts and returning NETDEV_TX_BUSY
is a driver bug in and out of itself: it means that our start/stop
queue flow control is broken.
This patch only fixes the (buggy) error handling code; the root
cause shall be fixed in a later commit.
Signed-off-by: Ahmed S. Darwish <ahmed.darwish@valeo.com>
---
drivers/net/can/usb/kvaser_usb.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
(Marc, Greg, I believe this should also be added to -stable?)
diff --git a/drivers/net/can/usb/kvaser_usb.c b/drivers/net/can/usb/kvaser_usb.c
index 541fb7a..6479a2b 100644
--- a/drivers/net/can/usb/kvaser_usb.c
+++ b/drivers/net/can/usb/kvaser_usb.c
@@ -1294,12 +1294,14 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
if (!urb) {
netdev_err(netdev, "No memory left for URBs\n");
stats->tx_dropped++;
- goto nourbmem;
+ dev_kfree_skb(skb);
+ return NETDEV_TX_OK;
}
buf = kmalloc(sizeof(struct kvaser_msg), GFP_ATOMIC);
if (!buf) {
stats->tx_dropped++;
+ dev_kfree_skb(skb);
goto nobufmem;
}
@@ -1334,6 +1336,9 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
}
}
+ /*
+ * This should never happen; it implies a flow control bug.
+ */
if (!context) {
netdev_warn(netdev, "cannot find free context\n");
ret = NETDEV_TX_BUSY;
@@ -1364,9 +1369,6 @@ static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
if (unlikely(err)) {
can_free_echo_skb(netdev, context->echo_index);
- skb = NULL; /* set to NULL to avoid double free in
- * dev_kfree_skb(skb) */
-
atomic_dec(&priv->active_tx_urbs);
usb_unanchor_urb(urb);
@@ -1388,8 +1390,6 @@ releasebuf:
kfree(buf);
nobufmem:
usb_free_urb(urb);
-nourbmem:
- dev_kfree_skb(skb);
return ret;
}
^ permalink raw reply related
* Re: [PATCH iproute2 v3] tc: Show classes in tree view
From: Stephen Hemminger @ 2014-12-24 20:42 UTC (permalink / raw)
To: Vadim Kochan; +Cc: netdev
In-Reply-To: <1419381976-23986-1-git-send-email-vadim4j@gmail.com>
On Wed, 24 Dec 2014 02:46:16 +0200
Vadim Kochan <vadim4j@gmail.com> wrote:
> From: Vadim Kochan <vadim4j@gmail.com>
>
> Added new '-t[ree]' which shows classes dependency
> in the tree view. Meanwhile only generic stats info
> is supported.
>
I don't get strict about checkpatch. But ran this patch through and there are some minor
things that would be good to fix.
1. Split long lines like:
WARNING: line over 80 characters
#198: FILE: tc/tc_class.c:219:
+static void tree_cls_show(FILE *fp, char *buf, struct hlist_head *root_list, int level)
2. Don't use variable name 'childs', makes sense to call it children instead??
3. Don't use space before * as in:
ERROR: "foo * bar" should be "foo *bar"
#285: FILE: tc/tc_class.c:305:
+ struct rtattr * tb[TCA_MAX+1] = {};
^ permalink raw reply
* [PATCH iproute2] ip: extend "ip-address" man page to reflect the recent flag extensions
From: Heiner Kallweit @ 2014-12-24 22:11 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev
Extend "ip-address" man page to reflect the recent extension of
allowing to list addresses with flags tentative, deprecated, dadfailed
not being set.
Signed-off-by: Heiner Kallweit <heiner.kallweit@web.de>
---
man/man8/ip-address.8.in | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/man/man8/ip-address.8.in b/man/man8/ip-address.8.in
index 3cfc56a..d33b1ed 100644
--- a/man/man8/ip-address.8.in
+++ b/man/man8/ip-address.8.in
@@ -50,8 +50,9 @@ ip-address \- protocol address management
.ti -8
.IR FLAG " := "
-.RB "[ " permanent " | " dynamic " | " secondary " | " primary " | "\
-tentative " | " deprecated " | " dadfailed " | " temporary " ]"
+.RB "[ " permanent " | " dynamic " | " secondary " | " primary " | \
+[ - ] " tentative " | [ - ] " deprecated " | [ - ] " dadfailed " | "\
+temporary " ]"
.SH "DESCRIPTION"
The
@@ -178,15 +179,29 @@ addresses.
address detection.
.TP
+.B -tentative
+(IPv6 only) only list addresses which are not in the process of
+duplicate address detection currently.
+
+.TP
.B deprecated
(IPv6 only) only list deprecated addresses.
.TP
+.B -deprecated
+(IPv6 only) only list addresses not being deprecated.
+
+.TP
.B dadfailed
(IPv6 only) only list addresses which have failed duplicate
address detection.
.TP
+.B -dadfailed
+(IPv6 only) only list addresses which have not failed duplicate
+address detection.
+
+.TP
.B temporary
(IPv6 only) only list temporary addresses.
--
2.2.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox