Re: [PATCH 2/4] misc: xgene: Add base driver for APM X-Gene SoC QueueManager/Traffic Manager

From: Ravi Patel
Date: Sat Dec 21 2013 - 19:20:48 EST


On Thu, Dec 19, 2013 at 7:20 PM, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Dec 19, 2013 at 06:45:01PM -0800, Ravi Patel wrote:
>> --- /dev/null
>> +++ b/drivers/misc/xgene/qmtm/Kconfig
>> @@ -0,0 +1,8 @@
>> +config XGENE_QMTM
>> + tristate "APM X-Gene QMTM driver"
>> + depends on ARCH_XGENE
>
> What does it need from this arch in order to build? What would be
> needed to get it to build on other arches so that it gets some actualy
> build testing?

In patch v2 added, depends on ARM64 || COMPILE_TEST in order to
support build testing for other arches.
If you think we should have no dependency at all, let us know.

>> --- /dev/null
>> +++ b/drivers/misc/xgene/qmtm/xgene_qmtm_main.c
>> @@ -0,0 +1,765 @@
>> +/*
>> + * AppliedMicro X-Gene SOC Queue Manager/Traffic Manager driver
>> + *
>> + * Copyright (c) 2013 Applied Micro Circuits Corporation.
>> + * Author: Ravi Patel <rapatel@xxxxxxx>
>> + * Keyur Chudgar <kchudgar@xxxxxxx>
>> + *
>> + * 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.
>
> Are you _sure_ about "any later version"? I have to ask.

In patch v2, removed "any later version" from this phrase.

>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>
> No need for this paragraph, the kernel has a copy of the GPL in it
> already.

Removed the paragraph from the license banner in patch v2.

>> +/* CSR Address Macros */
>> +#define CSR_QM_CONFIG_ADDR 0x00000004
>> +#define QM_ENABLE_WR(src) (((u32)(src)<<31) & 0x80000000)
>> +
>> +#define CSR_PBM_ADDR 0x00000008
>> +#define OVERWRITE_WR(src) (((u32)(src)<<31) & 0x80000000)
>> +#define SLVID_PBN_WR(src) (((u32)(src)) & 0x000003ff)
>> +#define SLAVE_ID_SHIFT 6
>
> Interesting way to align #defines, that's going to break badly over
> time, please just left align them like the rest of the kernel, using
> tabs.

Fixed the macro alignment to left align using tabs in patch v2.

>> +EXPORT_SYMBOL(xgene_qmtm_set_qinfo);
>
> EXPORT_SYMBOL_GPL() for this and the other exports perhaps? (sorry, I
> have to ask.)

Changed export to EXPORT_SYMBOL_GPL in patch v2.

>> +/*
>> + * Physical or free pool queue state (pq or fp)
>> + */
>> +struct storm_qmtm_pq_fp_qstate {
>> + /* word 0 31:0 */
>> + u32 resize_qid:10;
>> + u32 resize_start:1;
>> + u32 resize_done:1;
>> + u32 cfgtmvqen:1; /* enable pq to belong to vq */
>> + u32 cfgtmvq:10; /* parent vq */
>> + u32 cfgsaben:1; /* enable SAB broadcasting */
>> + u32 cpu_notify:8; /* cfgcpusab */
>> +
>> + /* word 1 63:32 */
>> + u32 cfgnotifyqne:1; /* enable queue not empty interrupt */
>> + u32 nummsg:16;
>> + u32 headptr:15;
>> +
>> + /* word 2 95:64 */
>> + u32 cfgcrid:1;
>> + u32 rid:3;
>> + u32 qcoherent:1;
>> + u64 cfgstartaddr:34; /* 27/7 split */
>> +
>> + /* word 3 127:96 */
>> + u32 vc_chanctl:2;
>> + u32 vc_chanid:2;
>> + u32 slot_pending:6;
>> + u32 stashing:1;
>> + u32 reserved_0:1;
>> + u32 cfgacceptlerr:1;
>> + u32 fp_mode:3; /* free pool mode */
>> + u32 cfgqsize:3; /* queue size, see xgene_qmtm_qsize */
>> + u32 qstatelock:1;
>> + u32 cfgRecombBuf:1;
>> + u32 cfgRecombBufTimeoutL:4; /* 4/3 split */
>> +
>> + /* word 4 159:128 */
>> + u32 cfgRecombBufTimeoutH:3; /* 4/3 split */
>> + u32 cfgselthrsh:3; /* associated threshold set */
>> + u32 resv2:13;
>> + u32 cfgqtype:2; /* queue type, refer xgene_qmtm_qtype */
>> + u32 resv1:11;
>> +} __packed;
>
> u64 for a bitfield? Is that even valid C? This is pretty scary from a
> bitfield perspective, what about different endian and addressing modes?

Currently the patch is supporting only ARM64 LE mode.
We will fix u64 bit-field with multiple u32 bit-fields for supporting
ARM32 LE Mode.

> Any chance to just use shifts to access the fields properly, like most
> drivers do, in a endian-neutral way to get the data out and into the
> structures?
>
> Same goes for the other structures in this file.

I agree with you that code with bit-mask and bit-shift takes care for
endian-neutral mode.
But QMTM in BE Mode, behaves differently by expecting message format
in memory as follow:
Swapping at 128 bits level.
a. For each 16 bytes, group of 4 bytes needs be swapped Higher to
lower <-> lower to higher,
b. As well as each bytes/bit fields in the group of 4 bytes also needs
to be swapped.
So because of 128 bit level swap, there will be complete different
code for bit-mask, bit-shift for BE and LE
So to support ARM64 BE and ARM32 BE, we are planning to just
re-arrange bit-fields reversely in the structures for BE and LE.

Regards,
Ravi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/