* Re: Badness with the kernel version 2.6.35-rc1-git1 running on P6 box
From: David Miller @ 2010-07-18 21:51 UTC (permalink / raw)
To: eric.dumazet
Cc: sachinp, netdev, linux-kernel, linuxppc-dev, ossthema, dipraksh
In-Reply-To: <1279282842.2549.16.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 16 Jul 2010 14:20:42 +0200
> Le vendredi 16 juillet 2010 =E0 11:56 +0200, Eric Dumazet a =E9crit :=
> =
>> [PATCH] ehea: ehea_get_stats() should use GFP_KERNEL
>> =
>> ehea_get_stats() is called in process context and should use GFP_KER=
NEL
>> allocation instead of GFP_ATOMIC.
>> =
>> Clearing stats at beginning of ehea_get_stats() is racy in case of
>> concurrent stat readers.
>> =
>> get_stats() can also use netdev net_device_stats, instead of a priva=
te
>> copy.
>> =
>> Reported-by: divya <dipraksh@linux.vnet.ibm.com>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>> drivers/net/ehea/ehea.h | 1 -
>> drivers/net/ehea/ehea_main.c | 6 ++----
>> 2 files changed, 2 insertions(+), 5 deletions(-)
>> =
>> =
> =
> Hmm, net-next-2.6 contains following patch :
If people think ehea usage is ubiquitous enough to deserve a backport
of this to net-2.6, fine. But personally I don't think it's worth it.
Can someone close the kernel bugzilla 16406 created for this bug? This=
patch we have already obviously would fix this issue.
^ permalink raw reply
* RE: [PATCH v2]460EX on-chip SATA driver<resubmisison>
From: Rupjyoti Sarmah @ 2010-07-18 17:33 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: linux-ide, sr, jgarzik, linux-kernel, linuxppc-dev
In-Reply-To: <4C41D174.2040907@ru.mvista.com>
Hi Sergei,
Thanks for your suggestions.
I would look into them and submit a patch for style improvement some time
later.
Regards,
Rup
-----Original Message-----
From: Sergei Shtylyov [mailto:sshtylyov@mvista.com]
Sent: Saturday, July 17, 2010 9:21 PM
To: Rupjyoti Sarmah
Cc: linux-ide@vger.kernel.org; linux-kernel@vger.kernel.org;
rsarmah@apm.com; jgarzik@pobox.com; sr@denx.de; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH v2]460EX on-chip SATA driver<resubmisison>
Hello.
Rupjyoti Sarmah wrote:
> This patch enables the on-chip DWC SATA controller of the AppliedMicro
processor 460EX.
Too bad thius has already been applied but here's my (mostly
stylistic)
comments anyway:
> Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
> Signed-off-by: Mark Miesfeld <mmiesfeld@appliedmicro.com>
> Signed-off-by: Prodyut Hazarika <phazarika@appliedmicro.com>
[...]
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> new file mode 100644
> index 0000000..ea24c1e
> --- /dev/null
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -0,0 +1,1756 @@
> +/*
> + * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
> +#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
> +#define DEBUG
> +#endif
> +
> +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
> +#define VERBOSE_DEBUG
> +#define DEBUG_NCQ
> +#endif
[...]
> +/* SATA DMA driver Globals */
> +#define DMA_NUM_CHANS 1
> +#define DMA_NUM_CHAN_REGS 8
> +
> +/* SATA DMA Register definitions */
> +#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/
Please put a space before */.
> +struct ahb_dma_regs {
> + struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS];
> + struct dma_interrupt_regs interrupt_raw; /* Raw Interrupt
*/
> + struct dma_interrupt_regs interrupt_status; /* Interrupt
Status */
> + struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask
*/
> + struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear
*/
> + struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
> +#define DMA_CTL_BLK_TS(size) ((size) & 0x000000FFF) /* Blk
Transfer size */
> +#define DMA_CHANNEL(ch) (0x00000001 << (ch)) /* Select
channel */
> + /* Enable channel */
> +#define DMA_ENABLE_CHAN(ch) ((0x00000001 << (ch)) |
\
> + ((0x000000001 << (ch)) << 8))
> + /* Disable channel */
> +#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001 <<
(ch)) << 8))
What's the point of OR'ing with zero?
> +/*
> + * Commonly used DWC SATA driver Macros
> + */
> +#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
> + (host)->private_data)
> +#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
> + (ap)->host->private_data)
> +#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
> + (ap)->private_data)
> +#define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\
> + (qc)->ap->host->private_data)
> +#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
> + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
> +struct sata_dwc_host_priv {
> + void __iomem *scr_addr_sstatus;
> + u32 sata_dwc_sactive_issued ;
> + u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
> +static void sata_dwc_tf_dump(struct ata_taskfile *tf)
> +{
> + dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s
flags:"
> + "0x%lx device: %x\n", tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
> +/*
> + * Function: get_burst_length_encode
> + * arguments: datalength: length in bytes of data
> + * returns value to be programmed in register corrresponding to data
length
> + * This value is effectively the log(base 2) of the length
> + */
> +static int get_burst_length_encode(int datalength)
> +{
> + int items = datalength >> 2; /* div by 4 to get lword count */
> +
> + if (items >= 64)
> + return 5;
> +
> + if (items >= 32)
> + return 4;
> +
> + if (items >= 16)
> + return 3;
> +
> + if (items >= 8)
> + return 2;
> +
> + if (items >= 4)
> + return 1;
> +
> + return 0;
> +}
Hmm, there should be a function in the kernel to calculate 2^n order
from
size, something like get_count_order()...
> +/*
> + * Function: dma_dwc_interrupt
> + * arguments: irq, dev_id, pt_regs
> + * returns channel number if available else -1
> + * Interrupt Handler for DW AHB SATA DMA
> + */
> +static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
> +{
> + int chan;
> + u32 tfr_reg, err_reg;
> + unsigned long flags;
> + struct sata_dwc_device *hsdev =
> + (struct sata_dwc_device *)hsdev_instance;
> + struct ata_host *host = (struct ata_host *)hsdev->host;
> + struct ata_port *ap;
> + struct sata_dwc_device_port *hsdevp;
> + u8 tag = 0;
> + unsigned int port = 0;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + ap = host->ports[port];
> + hsdevp = HSDEVP_FROM_AP(ap);
> + tag = ap->link.active_tag;
> +
> + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
> + .low));
> + err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
> + for (chan = 0; chan < DMA_NUM_CHANS; chan++) {
> + /* Check for end-of-transfer interrupt. */
> + if (tfr_reg & DMA_CHANNEL(chan)) {
> + /*
> + * Each DMA command produces 2 interrupts. Only
> + * complete the command after both interrupts have
been
> + * seen. (See sata_dwc_isr())
> + */
> + host_pvt.dma_interrupt_count++;
> + sata_dwc_clear_dmacr(hsdevp, tag);
> +
> + if (hsdevp->dma_pending[tag] ==
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "DMA not pending
eot=0x%08x "
> + "err=0x%08x tag=0x%02x
pending=%d\n",
> + tfr_reg, err_reg, tag,
> + hsdevp->dma_pending[tag]);
> + }
> +
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Prens around % operation not necessary.
> + sata_dwc_dma_xfer_complete(ap, 1);
> +
> + /* Clear the interrupt */
> +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> + /* Clear the interrupt. */
> +
out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> +/*
> + * Function: map_sg_to_lli
> + * The Synopsis driver has a comment proposing that better performance
> + * is possible by only enabling interrupts on the last item in the
linked list.
> + * However, it seems that could be a problem if an error happened on
one of the
> + * first items. The transfer would halt, but no error interrupt would
occur.
> + * Currently this function sets interrupts enabled for each linked list
item:
> + * DMA_CTL_INT_EN.
> + */
> +static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *dmadr_addr, int dir)
> +{
[...]
> + /* Program the LLI CTL high register */
> + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
> + (len / 4));
> +
> + /* Program the next pointer. The next pointer
must be
> + * the physical address, not the virtual address.
> + */
> + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
> +static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *addr, int dir)
> +{
> + int dma_ch;
> + int num_lli;
There should be an empty line here.
> +/*
> + * Function: dma_dwc_init
> + * arguments: hsdev
> + * returns status
> + * This function initializes the SATA DMA driver
> + */
> +static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
> +{
> + int err;
> +
> + err = dma_request_interrupts(hsdev, irq);
Could be initilaizer...
> + dev_notice(host_pvt.dwc_dev, "DMA initialized\n");
> + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
> +static u32 core_scr_read(unsigned int scr)
> +{
> + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
> +static void clear_serror(void)
> +{
> + u32 val;
Empty line needed here.
> + val = core_scr_read(SCR_ERROR);
This could be an initilaizer.
> +/* See ahci.c */
> +static void sata_dwc_error_intr(struct ata_port *ap,
> + struct sata_dwc_device *hsdev, uint intpr)
> +{
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct ata_eh_info *ehi = &ap->link.eh_info;
> + unsigned int err_mask = 0, action = 0;
> + struct ata_queued_cmd *qc;
> + u32 serror;
> + u8 status, tag;
> + u32 err_reg;
> +
> + ata_ehi_clear_desc(ehi);
> +
> + serror = core_scr_read(SCR_ERROR);
> + status = ap->ops->sff_check_status(ap);
Why not call ata_sff_check_status() directly?
> + err_reg =
in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
> +/*
> + * Function : sata_dwc_isr
> + * arguments : irq, void *dev_instance, struct pt_regs *regs
> + * Return value : irqreturn_t - status of IRQ
> + * This Interrupt handler called via port ops registered function.
> + * .irq_handler = sata_dwc_isr
> + */
> +static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
> +{
> + struct ata_host *host = (struct ata_host *)dev_instance;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_HOST(host);
> + struct ata_port *ap;
> + struct ata_queued_cmd *qc;
> + unsigned long flags;
> + u8 status, tag;
> + int handled, num_processed, port = 0;
> + uint intpr, sactive, sactive2, tag_mask;
> + struct sata_dwc_device_port *hsdevp;
> + host_pvt.sata_dwc_sactive_issued = 0;
[...]
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + /* If no sactive issued and tag_mask is zero then this is not NCQ
*/
> + if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0) {
> + if (ap->link.active_tag == ATA_TAG_POISON)
> + tag = 0;
> + else
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> +
> + /* DEV interrupt w/ no active qc? */
> + if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
> + dev_err(ap->dev, "%s interrupt with no active qc "
> + "qc=%p\n", __func__, qc);
> + ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly...
> + handled = 1;
> + goto DONE;
> + }
> + status = ap->ops->sff_check_status(ap);
Here too...
> +DRVSTILLBUSY:
> + if (ata_is_dma(qc->tf.protocol)) {
> + /*
> + * Each DMA transaction produces 2 interrupts. The
DMAC
> + * transfer complete interrupt and the SATA
controller
> + * operation done interrupt. The command should be
> + * completed only after both interrupts are seen.
> + */
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "%s: DMA not pending "
> + "intpr=0x%08x status=0x%08x
pending"
> + "=%d\n", __func__, intpr, status,
> + hsdevp->dma_pending[tag]);
> + }
Curly brace not needed here. I assume you haven't run the patch thru
scripts/checkpatch.pl?
> + /*
> + * This is a NCQ command. At this point we need to figure out for
which
> + * tags we have gotten a completion interrupt. One interrupt may
serve
> + * as completion for more than one operation when commands are
queued
> + * (NCQ). We need to process each completed command.
> + */
> +
> + /* process completed commands */
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not
needed.
> + tag_mask > 1) {
> + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x
sactive_issued=0x%08x"
> + "tag_mask=0x%08x\n", __func__, sactive,
> + host_pvt.sata_dwc_sactive_issued, tag_mask);
> + }
Curly braces not needed here.
> + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> +
(host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
> + dev_warn(ap->dev, "Bad tag mask? sactive=0x%08x "
> + "(host_pvt.sata_dwc_sactive_issued)=0x%08x
tag_mask"
> + "=0x%08x\n", sactive,
host_pvt.sata_dwc_sactive_issued,
> + tag_mask);
> + }
Curly braces not needed around single statement.
> +
> + /* read just to clear ... not bad if currently still busy */
> + status = ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly.
> + dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__,
status);
> +
> + tag = 0;
> + num_processed = 0;
> + while (tag_mask) {
> + num_processed++;
> + while (!(tag_mask & 0x00000001)) {
> + tag++;
> + tag_mask <<= 1;
> + }
> +
> + tag_mask &= (~0x00000001);
Useless parens.
> + /* Process completed command */
> + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n",
__func__,
> + ata_get_cmd_descript(qc->tf.protocol));
> + if (ata_is_dma(qc->tf.protocol)) {
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
\ not needed.
> + SATA_DWC_DMA_PENDING_NONE)
> + dev_warn(ap->dev, "%s: DMA not
pending?\n",
> + __func__);
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Parens not needed around % operation.
> + sata_dwc_dma_xfer_complete(ap, 1);
> + } else {
> + if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
> + goto STILLBUSY;
> + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}
> + /*
> + * Check to see if any commands completed while we were processing
our
> + * initial set of completed commands (read status clears
interrupts,
> + * so we might miss a completed command interrupt if one came in
while
> + * we were processing --we read status as part of processing a
completed
> + * command).
> + */
> + sactive2 = core_scr_read(SCR_ACTIVE);
> + if (sactive2 != sactive) {
{} not needed here.
> +static void sata_dwc_dma_xfer_complete(struct ata_port *ap, u32
check_status)
> +{
> + struct ata_queued_cmd *qc;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + u8 tag = 0;
> +
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> + if (!qc) {
> + dev_err(ap->dev, "failed to get qc");
> + return;
> + }
> +
> +#ifdef DEBUG_NCQ
> + if (tag > 0) {
Curly braces not needed around single statement.
> + dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s
proto=%s "
> + "dmacr=0x%08x\n", __func__, qc->tag,
qc->tf.command,
> + ata_get_cmd_descript(qc->dma_dir),
> + ata_get_cmd_descript(qc->tf.protocol),
> + in_le32(&(hsdev->sata_dwc_regs->dmacr)));
> + }
> +#endif
> +
> + if (ata_is_dma(qc->tf.protocol)) {
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_NONE)
{
Curly braces not needed around single statement.
> +static int sata_dwc_qc_complete(struct ata_port *ap, struct
ata_queued_cmd *qc,
> + u32 check_status)
> +{
> + u8 status = 0;
> + u32 mask = 0x0;
> + u8 tag = qc->tag;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
There should be an empty line here.
> + host_pvt.sata_dwc_sactive_queued = 0;
> + dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__, check_status);
> +
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
> + dev_err(ap->dev, "TX DMA PENDING\n");
> + else if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_RX)
> + dev_err(ap->dev, "RX DMA PENDING\n");
> + dev_dbg(ap->dev, "QC complete cmd=0x%02x status=0x%02x ata%u:"
> + " protocol=%d\n", qc->tf.command, status, ap->print_id,
> + qc->tf.protocol);
> +
> + /* clear active bit */
> + mask = (~(qcmd_tag_to_mask(tag)));
Useless parens.
> + host_pvt.sata_dwc_sactive_queued =
(host_pvt.sata_dwc_sactive_queued) \
> + & mask;
> + host_pvt.sata_dwc_sactive_issued =
(host_pvt.sata_dwc_sactive_issued) \
\ not needed.
> +static void sata_dwc_port_stop(struct ata_port *ap)
> +{
> + int i;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> +
> + dev_dbg(ap->dev, "%s: ap->id = %d\n", __func__, ap->print_id);
> +
> + if (hsdevp && hsdev) {
> + /* deallocate LLI table */
> + for (i = 0; i < SATA_DWC_QCMD_MAX; i++) {
Curly braces not needed.
> + dma_free_coherent(ap->host->dev,
> + SATA_DWC_DMAC_LLI_TBL_SZ,
> + hsdevp->llit[i],
hsdevp->llit_dma[i]);
Should be indented on the same level as above line
> +static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8
tag)
> +{
> + int start_dma;
> + u32 reg, dma_chan;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_QC(qc);
> + struct ata_port *ap = qc->ap;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + int dir = qc->dma_dir;
There should be an empty line here.
> + if (start_dma) {
> + reg = core_scr_read(SCR_ERROR);
> + if (reg & SATA_DWC_SERROR_ERR_BITS) {
Curly braces not needed here.
> +static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x
tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +/*
> + * Function : sata_dwc_qc_prep_by_tag
> + * arguments : ata_queued_cmd *qc, u8 tag
> + * Return value : None
> + * qc_prep for a particular queued command based on tag
> + */
> +static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
[...]
> + dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
> + hsdevp->llit_dma[tag],
> + (void
*__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
> +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> +{
> + u32 sactive;
> + u8 tag = qc->tag;
> + struct ata_port *ap = qc->ap;
[...]
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + sactive = core_scr_read(SCR_ACTIVE);
> + sactive |= (0x00000001 << tag);
Parens not needed.
> + core_scr_write(SCR_ACTIVE, sactive);
> +
> + dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x
"
> + "sactive=0x%08x\n", __func__, tag,
qc->ap->link.sactive,
> + sactive);
> +
> + ap->ops->sff_tf_load(ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
> +/*
> + * Function : sata_dwc_qc_prep
> + * arguments : ata_queued_cmd *qc
> + * Return value : None
> + * qc_prep for a particular queued command
> + */
> +
> +static void sata_dwc_qc_prep(struct ata_queued_cmd *qc)
> +{
> + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol ==
ATA_PROT_PIO))
Parens not needed arounf == operation.
> + return;
> +
> +#ifdef DEBUG_NCQ
> + if (qc->tag > 0)
> + dev_info(qc->ap->dev, "%s: qc->tag=%d
ap->active_tag=0x%08x\n",
> + __func__, tag, qc->ap->link.active_tag);
> +
> + return ;
Remove spade before semicolon please.
> +static const struct ata_port_info sata_dwc_port_info[] = {
> + {
> + .flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> + ATA_FLAG_MMIO | ATA_FLAG_NCQ,
> + .pio_mask = 0x1f, /* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
> +static int sata_dwc_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
> +{
> + struct sata_dwc_device *hsdev;
> + u32 idr, versionr;
> + char *ver = (char *)&versionr;
> + u8 *base = NULL;
> + int err = 0;
> + int irq, rc;
> + struct ata_host *host;
> + struct ata_port_info pi = sata_dwc_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + /* Allocate DWC SATA device */
> + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
> + if (hsdev == NULL) {
> + dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> + err = -ENOMEM;
> + goto error_out;
> + }
> + memset(hsdev, 0, sizeof(*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
> + /* Get physical SATA DMA register base address */
> + host_pvt.sata_dma_regs = of_iomap(ofdev->dev.of_node, 1);
> + if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
> + /*
> + * Now, register with libATA core, this will also initiate the
> + * device discovery process, invoking our port_start() handler &
> + * error_handler() to execute a dummy Softreset EH session
> + */
> + rc = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> +
I think that empty line here is not needed.
> + if (rc != 0)
> + dev_err(&ofdev->dev, "failed to activate host");
> +
> + dev_set_drvdata(&ofdev->dev, host);
> + return 0;
> +
> +error_out:
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + if (base)
> + iounmap(base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
> +static int sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
> +{
> + struct device *dev = &ofdev->dev;
> + struct ata_host *host = dev_get_drvdata(dev);
> + struct sata_dwc_device *hsdev = host->private_data;
> +
> + ata_host_detach(host);
> + dev_set_drvdata(dev, NULL);
> +
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + iounmap(hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
> + kfree(hsdev);
> + kfree(host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: Artem Bityutskiy @ 2010-07-18 16:52 UTC (permalink / raw)
To: Josh Boyer
Cc: Randy Dunlap, linuxppc-dev, Alexander Kurz, Paul Mackerras,
John Linn, David Brown, Ladislav Michl, Solomon Peachy, vamos-dev,
Mike Frysinger, Florian Fainelli, Artem Bityutskiy,
Christian Dietrich, Nicolas Pitre, Jiri Kosina, linux-kernel,
Milton Miller, netdev, Joe Perches, linux-mtd, David Woodhouse,
David S. Miller
In-Reply-To: <20100716142055.GA11736@zod.rchland.ibm.com>
On Fri, 2010-07-16 at 10:20 -0400, Josh Boyer wrote:
> On Fri, Jul 16, 2010 at 02:29:02PM +0200, Christian Dietrich wrote:
> >The config options for REDWOOD_[456] were commented out in the powerpc
> >Kconfig. The ifdefs referencing this options therefore are dead and all
> >references to this can be removed (Also dependencies in other KConfig
> >files).
> >
> >Signed-off-by: Christian Dietrich <qy03fugy@stud.informatik.uni-erlangen.de>
> >Signed-off-by: Christoph Egger <siccegge@cs.fau.de>
>
> This seems fine with me.
>
> The only question is which tree it coms through. I'm happy to take it
> in via mine if the netdev and MTD people are fine with that. Otherwise,
> my ack is below.
>
> Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
You know how slow MTD people may be sometimes, so I'd suggest you to
merge this via whatever tree. David is in CC, he'll complain if he is
unhappy, I think.
--
Best Regards,
Artem Bityutskiy (Артём Битюцкий)
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Segher Boessenkool @ 2010-07-17 16:41 UTC (permalink / raw)
To: Grant Likely
Cc: Kumar Gala, Mitch Bradley, Matthew McClintock, linuxppc-dev,
Timur Tabi, David Gibson
In-Reply-To: <AANLkTilDcYYbsH-6f0_HlX9WwaOwG24w1CfhRLplZKS7@mail.gmail.com>
>>>> Yes. Where would we get a list of memreserve sections?
>>>
>>> I would say the list of reserves that are not under the control of
>>> Linux should be explicitly described in the device tree proper. For
>>> instance, if you have a region that firmware depends on, then have a
>>> node for describing the firmware and a property stating the memory
>>> regions that it depends on. The memreserve regions can be generated
>>> from that.
>>
>> Ok, so we could traverse the tree node-by-bode for a
>> persistent-memreserve property and add them to the /memreserve/
>> list in
>> the kexec user space tools?
>
> I *think* that is okay, but I'd like to hear from Segher, Ben, Mitch,
> David Gibson, and other device tree experts on whether or not that
> exact property naming is a good one.
Let's take a step back: what exactly _are_ "memreserve sections", what
are they used for, who (kernel, firmware, bootloader, etc.) holds what
responsibility wrt them?
Segher
^ permalink raw reply
* Re: [PATCH v2]460EX on-chip SATA driver<resubmisison>
From: Sergei Shtylyov @ 2010-07-17 15:51 UTC (permalink / raw)
To: Rupjyoti Sarmah
Cc: linux-ide, rsarmah, linux-kernel, linuxppc-dev, sr, jgarzik
In-Reply-To: <201007061106.o66B631f013777@amcc.com>
Hello.
Rupjyoti Sarmah wrote:
> This patch enables the on-chip DWC SATA controller of the AppliedMicro processor 460EX.
Too bad thius has already been applied but here's my (mostly stylistic)
comments anyway:
> Signed-off-by: Rupjyoti Sarmah <rsarmah@appliedmicro.com>
> Signed-off-by: Mark Miesfeld <mmiesfeld@appliedmicro.com>
> Signed-off-by: Prodyut Hazarika <phazarika@appliedmicro.com>
[...]
> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> new file mode 100644
> index 0000000..ea24c1e
> --- /dev/null
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -0,0 +1,1756 @@
> +/*
> + * drivers/ata/sata_dwc_460ex.c
Filenames in the heading comments have long been frowned upon.
> +#ifdef CONFIG_SATA_DWC_DEBUG
I don't see this option defined anywahere.
> +#define DEBUG
> +#endif
> +
> +#ifdef CONFIG_SATA_DWC_VDEBUG
The same about this one.
> +#define VERBOSE_DEBUG
> +#define DEBUG_NCQ
> +#endif
[...]
> +/* SATA DMA driver Globals */
> +#define DMA_NUM_CHANS 1
> +#define DMA_NUM_CHAN_REGS 8
> +
> +/* SATA DMA Register definitions */
> +#define AHB_DMA_BRST_DFLT 64 /* 16 data items burst length*/
Please put a space before */.
> +struct ahb_dma_regs {
> + struct dma_chan_regs chan_regs[DMA_NUM_CHAN_REGS];
> + struct dma_interrupt_regs interrupt_raw; /* Raw Interrupt */
> + struct dma_interrupt_regs interrupt_status; /* Interrupt Status */
> + struct dma_interrupt_regs interrupt_mask; /* Interrupt Mask */
> + struct dma_interrupt_regs interrupt_clear; /* Interrupt Clear */
> + struct dmareg statusInt; /* Interrupt combined*/
No camelCase please, rename it to status_int.
> +#define DMA_CTL_BLK_TS(size) ((size) & 0x000000FFF) /* Blk Transfer size */
> +#define DMA_CHANNEL(ch) (0x00000001 << (ch)) /* Select channel */
> + /* Enable channel */
> +#define DMA_ENABLE_CHAN(ch) ((0x00000001 << (ch)) | \
> + ((0x000000001 << (ch)) << 8))
> + /* Disable channel */
> +#define DMA_DISABLE_CHAN(ch) (0x00000000 | ((0x000000001 << (ch)) << 8))
What's the point of OR'ing with zero?
> +/*
> + * Commonly used DWC SATA driver Macros
> + */
> +#define HSDEV_FROM_HOST(host) ((struct sata_dwc_device *)\
> + (host)->private_data)
> +#define HSDEV_FROM_AP(ap) ((struct sata_dwc_device *)\
> + (ap)->host->private_data)
> +#define HSDEVP_FROM_AP(ap) ((struct sata_dwc_device_port *)\
> + (ap)->private_data)
> +#define HSDEV_FROM_QC(qc) ((struct sata_dwc_device *)\
> + (qc)->ap->host->private_data)
> +#define HSDEV_FROM_HSDEVP(p) ((struct sata_dwc_device *)\
> + (hsdevp)->hsdev)
Are you sure it's '(hsdevp)', not '(p)'?
> +struct sata_dwc_host_priv {
> + void __iomem *scr_addr_sstatus;
> + u32 sata_dwc_sactive_issued ;
> + u32 sata_dwc_sactive_queued ;
Remove spaces befoer semicolons, please.
> +static void sata_dwc_tf_dump(struct ata_taskfile *tf)
> +{
> + dev_vdbg(host_pvt.dwc_dev, "taskfile cmd: 0x%02x protocol: %s flags:"
> + "0x%lx device: %x\n", tf->command, ata_get_cmd_descript\
There's no need to use \ outside macro defintions.
> +/*
> + * Function: get_burst_length_encode
> + * arguments: datalength: length in bytes of data
> + * returns value to be programmed in register corrresponding to data length
> + * This value is effectively the log(base 2) of the length
> + */
> +static int get_burst_length_encode(int datalength)
> +{
> + int items = datalength >> 2; /* div by 4 to get lword count */
> +
> + if (items >= 64)
> + return 5;
> +
> + if (items >= 32)
> + return 4;
> +
> + if (items >= 16)
> + return 3;
> +
> + if (items >= 8)
> + return 2;
> +
> + if (items >= 4)
> + return 1;
> +
> + return 0;
> +}
Hmm, there should be a function in the kernel to calculate 2^n order from
size, something like get_count_order()...
> +/*
> + * Function: dma_dwc_interrupt
> + * arguments: irq, dev_id, pt_regs
> + * returns channel number if available else -1
> + * Interrupt Handler for DW AHB SATA DMA
> + */
> +static irqreturn_t dma_dwc_interrupt(int irq, void *hsdev_instance)
> +{
> + int chan;
> + u32 tfr_reg, err_reg;
> + unsigned long flags;
> + struct sata_dwc_device *hsdev =
> + (struct sata_dwc_device *)hsdev_instance;
> + struct ata_host *host = (struct ata_host *)hsdev->host;
> + struct ata_port *ap;
> + struct sata_dwc_device_port *hsdevp;
> + u8 tag = 0;
> + unsigned int port = 0;
> +
> + spin_lock_irqsave(&host->lock, flags);
> + ap = host->ports[port];
> + hsdevp = HSDEVP_FROM_AP(ap);
> + tag = ap->link.active_tag;
> +
> + tfr_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.tfr\
There's no need to use \ outside macro defintions.
> + .low));
> + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error\
Same comment here...
> + for (chan = 0; chan < DMA_NUM_CHANS; chan++) {
> + /* Check for end-of-transfer interrupt. */
> + if (tfr_reg & DMA_CHANNEL(chan)) {
> + /*
> + * Each DMA command produces 2 interrupts. Only
> + * complete the command after both interrupts have been
> + * seen. (See sata_dwc_isr())
> + */
> + host_pvt.dma_interrupt_count++;
> + sata_dwc_clear_dmacr(hsdevp, tag);
> +
> + if (hsdevp->dma_pending[tag] ==
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "DMA not pending eot=0x%08x "
> + "err=0x%08x tag=0x%02x pending=%d\n",
> + tfr_reg, err_reg, tag,
> + hsdevp->dma_pending[tag]);
> + }
> +
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Prens around % operation not necessary.
> + sata_dwc_dma_xfer_complete(ap, 1);
> +
> + /* Clear the interrupt */
> + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> + /* Clear the interrupt. */
> + out_le32(&(host_pvt.sata_dma_regs->interrupt_clear\
\ not needed.
> +/*
> + * Function: map_sg_to_lli
> + * The Synopsis driver has a comment proposing that better performance
> + * is possible by only enabling interrupts on the last item in the linked list.
> + * However, it seems that could be a problem if an error happened on one of the
> + * first items. The transfer would halt, but no error interrupt would occur.
> + * Currently this function sets interrupts enabled for each linked list item:
> + * DMA_CTL_INT_EN.
> + */
> +static int map_sg_to_lli(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *dmadr_addr, int dir)
> +{
[...]
> + /* Program the LLI CTL high register */
> + lli[idx].ctl.high = cpu_to_le32(DMA_CTL_BLK_TS\
\ not needed.
> + (len / 4));
> +
> + /* Program the next pointer. The next pointer must be
> + * the physical address, not the virtual address.
> + */
> + next_llp = (dma_lli + ((idx + 1) * sizeof(struct \
Same here...
> +static int dma_dwc_xfer_setup(struct scatterlist *sg, int num_elems,
> + struct lli *lli, dma_addr_t dma_lli,
> + void __iomem *addr, int dir)
> +{
> + int dma_ch;
> + int num_lli;
There should be an empty line here.
> +/*
> + * Function: dma_dwc_init
> + * arguments: hsdev
> + * returns status
> + * This function initializes the SATA DMA driver
> + */
> +static int dma_dwc_init(struct sata_dwc_device *hsdev, int irq)
> +{
> + int err;
> +
> + err = dma_request_interrupts(hsdev, irq);
Could be initilaizer...
> + dev_notice(host_pvt.dwc_dev, "DMA initialized\n");
> + dev_dbg(host_pvt.dwc_dev, "SATA DMA registers=0x%p\n", host_pvt.\
\ not needed.
> +static u32 core_scr_read(unsigned int scr)
> +{
> + return in_le32((void __iomem *)(host_pvt.scr_addr_sstatus) +\
Not needed.
> +static void clear_serror(void)
> +{
> + u32 val;
Empty line needed here.
> + val = core_scr_read(SCR_ERROR);
This could be an initilaizer.
> +/* See ahci.c */
> +static void sata_dwc_error_intr(struct ata_port *ap,
> + struct sata_dwc_device *hsdev, uint intpr)
> +{
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct ata_eh_info *ehi = &ap->link.eh_info;
> + unsigned int err_mask = 0, action = 0;
> + struct ata_queued_cmd *qc;
> + u32 serror;
> + u8 status, tag;
> + u32 err_reg;
> +
> + ata_ehi_clear_desc(ehi);
> +
> + serror = core_scr_read(SCR_ERROR);
> + status = ap->ops->sff_check_status(ap);
Why not call ata_sff_check_status() directly?
> + err_reg = in_le32(&(host_pvt.sata_dma_regs->interrupt_status.error.\
\ not neeeded.
> +/*
> + * Function : sata_dwc_isr
> + * arguments : irq, void *dev_instance, struct pt_regs *regs
> + * Return value : irqreturn_t - status of IRQ
> + * This Interrupt handler called via port ops registered function.
> + * .irq_handler = sata_dwc_isr
> + */
> +static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
> +{
> + struct ata_host *host = (struct ata_host *)dev_instance;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_HOST(host);
> + struct ata_port *ap;
> + struct ata_queued_cmd *qc;
> + unsigned long flags;
> + u8 status, tag;
> + int handled, num_processed, port = 0;
> + uint intpr, sactive, sactive2, tag_mask;
> + struct sata_dwc_device_port *hsdevp;
> + host_pvt.sata_dwc_sactive_issued = 0;
[...]
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + /* If no sactive issued and tag_mask is zero then this is not NCQ */
> + if (host_pvt.sata_dwc_sactive_issued == 0 && tag_mask == 0) {
> + if (ap->link.active_tag == ATA_TAG_POISON)
> + tag = 0;
> + else
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> +
> + /* DEV interrupt w/ no active qc? */
> + if (unlikely(!qc || (qc->tf.flags & ATA_TFLAG_POLLING))) {
> + dev_err(ap->dev, "%s interrupt with no active qc "
> + "qc=%p\n", __func__, qc);
> + ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly...
> + handled = 1;
> + goto DONE;
> + }
> + status = ap->ops->sff_check_status(ap);
Here too...
> +DRVSTILLBUSY:
> + if (ata_is_dma(qc->tf.protocol)) {
> + /*
> + * Each DMA transaction produces 2 interrupts. The DMAC
> + * transfer complete interrupt and the SATA controller
> + * operation done interrupt. The command should be
> + * completed only after both interrupts are seen.
> + */
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
> + SATA_DWC_DMA_PENDING_NONE) {
> + dev_err(ap->dev, "%s: DMA not pending "
> + "intpr=0x%08x status=0x%08x pending"
> + "=%d\n", __func__, intpr, status,
> + hsdevp->dma_pending[tag]);
> + }
Curly brace not needed here. I assume you haven't run the patch thru
scripts/checkpatch.pl?
> + /*
> + * This is a NCQ command. At this point we need to figure out for which
> + * tags we have gotten a completion interrupt. One interrupt may serve
> + * as completion for more than one operation when commands are queued
> + * (NCQ). We need to process each completed command.
> + */
> +
> + /* process completed commands */
> + sactive = core_scr_read(SCR_ACTIVE);
> + tag_mask = (host_pvt.sata_dwc_sactive_issued | sactive) ^ sactive;
Isn't it the same as 'host_pvt.sata_dwc_sactive_issued & ~sactive'?
> + if (sactive != 0 || (host_pvt.sata_dwc_sactive_issued) > 1 || \
Useless parens around 'host_pvt.sata_dwc_sactive_issued' + \ not needed.
> + tag_mask > 1) {
> + dev_dbg(ap->dev, "%s NCQ:sactive=0x%08x sactive_issued=0x%08x"
> + "tag_mask=0x%08x\n", __func__, sactive,
> + host_pvt.sata_dwc_sactive_issued, tag_mask);
> + }
Curly braces not needed here.
> + if ((tag_mask | (host_pvt.sata_dwc_sactive_issued)) != \
> + (host_pvt.sata_dwc_sactive_issued)) {
Useless parens around 'host_pvt.sata_dwc_sactive_issued'.
> + dev_warn(ap->dev, "Bad tag mask? sactive=0x%08x "
> + "(host_pvt.sata_dwc_sactive_issued)=0x%08x tag_mask"
> + "=0x%08x\n", sactive, host_pvt.sata_dwc_sactive_issued,
> + tag_mask);
> + }
Curly braces not needed around single statement.
> +
> + /* read just to clear ... not bad if currently still busy */
> + status = ap->ops->sff_check_status(ap);
Call ata_sff_check_status() directly.
> + dev_dbg(ap->dev, "%s ATA status register=0x%x\n", __func__, status);
> +
> + tag = 0;
> + num_processed = 0;
> + while (tag_mask) {
> + num_processed++;
> + while (!(tag_mask & 0x00000001)) {
> + tag++;
> + tag_mask <<= 1;
> + }
> +
> + tag_mask &= (~0x00000001);
Useless parens.
> + /* Process completed command */
> + dev_dbg(ap->dev, "%s NCQ command, protocol: %s\n", __func__,
> + ata_get_cmd_descript(qc->tf.protocol));
> + if (ata_is_dma(qc->tf.protocol)) {
> + host_pvt.dma_interrupt_count++;
> + if (hsdevp->dma_pending[tag] == \
\ not needed.
> + SATA_DWC_DMA_PENDING_NONE)
> + dev_warn(ap->dev, "%s: DMA not pending?\n",
> + __func__);
> + if ((host_pvt.dma_interrupt_count % 2) == 0)
Parens not needed around % operation.
> + sata_dwc_dma_xfer_complete(ap, 1);
> + } else {
> + if (unlikely(sata_dwc_qc_complete(ap, qc, 1)))
> + goto STILLBUSY;
> + }
You should write:
} else if (unlikely(sata_dwc_qc_complete(ap, qc, 1))) {
goto STILLBUSY;
}
> + /*
> + * Check to see if any commands completed while we were processing our
> + * initial set of completed commands (read status clears interrupts,
> + * so we might miss a completed command interrupt if one came in while
> + * we were processing --we read status as part of processing a completed
> + * command).
> + */
> + sactive2 = core_scr_read(SCR_ACTIVE);
> + if (sactive2 != sactive) {
{} not needed here.
> +static void sata_dwc_dma_xfer_complete(struct ata_port *ap, u32 check_status)
> +{
> + struct ata_queued_cmd *qc;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + u8 tag = 0;
> +
> + tag = ap->link.active_tag;
> + qc = ata_qc_from_tag(ap, tag);
> + if (!qc) {
> + dev_err(ap->dev, "failed to get qc");
> + return;
> + }
> +
> +#ifdef DEBUG_NCQ
> + if (tag > 0) {
Curly braces not needed around single statement.
> + dev_info(ap->dev, "%s tag=%u cmd=0x%02x dma dir=%s proto=%s "
> + "dmacr=0x%08x\n", __func__, qc->tag, qc->tf.command,
> + ata_get_cmd_descript(qc->dma_dir),
> + ata_get_cmd_descript(qc->tf.protocol),
> + in_le32(&(hsdev->sata_dwc_regs->dmacr)));
> + }
> +#endif
> +
> + if (ata_is_dma(qc->tf.protocol)) {
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_NONE) {
Curly braces not needed around single statement.
> +static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc,
> + u32 check_status)
> +{
> + u8 status = 0;
> + u32 mask = 0x0;
> + u8 tag = qc->tag;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
There should be an empty line here.
> + host_pvt.sata_dwc_sactive_queued = 0;
> + dev_dbg(ap->dev, "%s checkstatus? %x\n", __func__, check_status);
> +
> + if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_TX)
> + dev_err(ap->dev, "TX DMA PENDING\n");
> + else if (hsdevp->dma_pending[tag] == SATA_DWC_DMA_PENDING_RX)
> + dev_err(ap->dev, "RX DMA PENDING\n");
> + dev_dbg(ap->dev, "QC complete cmd=0x%02x status=0x%02x ata%u:"
> + " protocol=%d\n", qc->tf.command, status, ap->print_id,
> + qc->tf.protocol);
> +
> + /* clear active bit */
> + mask = (~(qcmd_tag_to_mask(tag)));
Useless parens.
> + host_pvt.sata_dwc_sactive_queued = (host_pvt.sata_dwc_sactive_queued) \
> + & mask;
> + host_pvt.sata_dwc_sactive_issued = (host_pvt.sata_dwc_sactive_issued) \
\ not needed.
> +static void sata_dwc_port_stop(struct ata_port *ap)
> +{
> + int i;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_AP(ap);
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> +
> + dev_dbg(ap->dev, "%s: ap->id = %d\n", __func__, ap->print_id);
> +
> + if (hsdevp && hsdev) {
> + /* deallocate LLI table */
> + for (i = 0; i < SATA_DWC_QCMD_MAX; i++) {
Curly braces not needed.
> + dma_free_coherent(ap->host->dev,
> + SATA_DWC_DMAC_LLI_TBL_SZ,
> + hsdevp->llit[i], hsdevp->llit_dma[i]);
Should be indented on the same level as above line
> +static void sata_dwc_bmdma_setup(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +static void sata_dwc_bmdma_start_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
> + int start_dma;
> + u32 reg, dma_chan;
> + struct sata_dwc_device *hsdev = HSDEV_FROM_QC(qc);
> + struct ata_port *ap = qc->ap;
> + struct sata_dwc_device_port *hsdevp = HSDEVP_FROM_AP(ap);
> + int dir = qc->dma_dir;
There should be an empty line here.
> + if (start_dma) {
> + reg = core_scr_read(SCR_ERROR);
> + if (reg & SATA_DWC_SERROR_ERR_BITS) {
Curly braces not needed here.
> +static void sata_dwc_bmdma_start(struct ata_queued_cmd *qc)
> +{
> + u8 tag = qc->tag;
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + dev_dbg(qc->ap->dev, "%s: ap->link.sactive=0x%08x tag=%d\n",
> + __func__, qc->ap->link.sactive, tag);
> + } else {
> + tag = 0;
> + }
Curly braces not needed around single statements.
> +/*
> + * Function : sata_dwc_qc_prep_by_tag
> + * arguments : ata_queued_cmd *qc, u8 tag
> + * Return value : None
> + * qc_prep for a particular queued command based on tag
> + */
> +static void sata_dwc_qc_prep_by_tag(struct ata_queued_cmd *qc, u8 tag)
> +{
[...]
> + dma_chan = dma_dwc_xfer_setup(sg, qc->n_elem, hsdevp->llit[tag],
> + hsdevp->llit_dma[tag],
> + (void *__iomem)(&hsdev->sata_dwc_regs->\
\ not needed.
> +static unsigned int sata_dwc_qc_issue(struct ata_queued_cmd *qc)
> +{
> + u32 sactive;
> + u8 tag = qc->tag;
> + struct ata_port *ap = qc->ap;
[...]
> +
> + if (ata_is_ncq(qc->tf.protocol)) {
> + sactive = core_scr_read(SCR_ACTIVE);
> + sactive |= (0x00000001 << tag);
Parens not needed.
> + core_scr_write(SCR_ACTIVE, sactive);
> +
> + dev_dbg(qc->ap->dev, "%s: tag=%d ap->link.sactive = 0x%08x "
> + "sactive=0x%08x\n", __func__, tag, qc->ap->link.sactive,
> + sactive);
> +
> + ap->ops->sff_tf_load(ap, &qc->tf);
Why not call ata_sff_tf_load() directly?
> +/*
> + * Function : sata_dwc_qc_prep
> + * arguments : ata_queued_cmd *qc
> + * Return value : None
> + * qc_prep for a particular queued command
> + */
> +
> +static void sata_dwc_qc_prep(struct ata_queued_cmd *qc)
> +{
> + if ((qc->dma_dir == DMA_NONE) || (qc->tf.protocol == ATA_PROT_PIO))
Parens not needed arounf == operation.
> + return;
> +
> +#ifdef DEBUG_NCQ
> + if (qc->tag > 0)
> + dev_info(qc->ap->dev, "%s: qc->tag=%d ap->active_tag=0x%08x\n",
> + __func__, tag, qc->ap->link.active_tag);
> +
> + return ;
Remove spade before semicolon please.
> +static const struct ata_port_info sata_dwc_port_info[] = {
> + {
> + .flags = ATA_FLAG_SATA | ATA_FLAG_NO_LEGACY |
> + ATA_FLAG_MMIO | ATA_FLAG_NCQ,
> + .pio_mask = 0x1f, /* pio 0-4 */
Replace 0x1f with ATA_PIO4, please.
> +static int sata_dwc_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
Shouldn't this function be '__init' or '__devinit'?
> +{
> + struct sata_dwc_device *hsdev;
> + u32 idr, versionr;
> + char *ver = (char *)&versionr;
> + u8 *base = NULL;
> + int err = 0;
> + int irq, rc;
> + struct ata_host *host;
> + struct ata_port_info pi = sata_dwc_port_info[0];
> + const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> + /* Allocate DWC SATA device */
> + hsdev = kmalloc(sizeof(*hsdev), GFP_KERNEL);
> + if (hsdev == NULL) {
> + dev_err(&ofdev->dev, "kmalloc failed for hsdev\n");
> + err = -ENOMEM;
> + goto error_out;
> + }
> + memset(hsdev, 0, sizeof(*hsdev));
Use kzalloc() instead iof kmalloc()/memset().
> + /* Get physical SATA DMA register base address */
> + host_pvt.sata_dma_regs = of_iomap(ofdev->dev.of_node, 1);
> + if (!(host_pvt.sata_dma_regs)) {
Parens not needed around 'host_pvt.sata_dma_regs'.
> + /*
> + * Now, register with libATA core, this will also initiate the
> + * device discovery process, invoking our port_start() handler &
> + * error_handler() to execute a dummy Softreset EH session
> + */
> + rc = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> +
I think that empty line here is not needed.
> + if (rc != 0)
> + dev_err(&ofdev->dev, "failed to activate host");
> +
> + dev_set_drvdata(&ofdev->dev, host);
> + return 0;
> +
> +error_out:
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + if (base)
> + iounmap(base);
What about freeing 'hsdev' and 'host' and unmapping
'host_pvt.sata_dma_regs'? And does iounmap() really pair with of_iomap()?
> +static int sata_dwc_remove(struct of_device *ofdev)
Shouldn't this be '__exit' or '__devexit'?
> +{
> + struct device *dev = &ofdev->dev;
> + struct ata_host *host = dev_get_drvdata(dev);
> + struct sata_dwc_device *hsdev = host->private_data;
> +
> + ata_host_detach(host);
> + dev_set_drvdata(dev, NULL);
> +
> + /* Free SATA DMA resources */
> + dma_dwc_exit(hsdev);
> +
> + iounmap(hsdev->reg_base);
What about unmapping 'host_pvt.sata_dma_regs'?
> + kfree(hsdev);
> + kfree(host);
Does kfree() really pair with ata_host_alloc_pinfo()?
MBR, Sergei
^ permalink raw reply
* unsubscribe
From: aiolos.cis90 @ 2010-07-17 11:30 UTC (permalink / raw)
To: linuxppc-dev
unsubscribe
^ permalink raw reply
* Re: Badness with the kernel version 2.6.35-rc1-git1 running on P6 box
From: Maciej Rutecki @ 2010-07-17 5:52 UTC (permalink / raw)
To: divya; +Cc: sachinp, linuxppc-dev, LKML
In-Reply-To: <4C401D56.3070108@linux.vnet.ibm.com>
On pi=C4=85tek, 16 lipca 2010 o 10:50:30 divya wrote:
> Hi ,
>=20
> With the latest kernel version 2.6.35-rc5-git1(2f7989efd4398) running on
> power(p6) box came across the following call trace
>=20
I created a Bugzilla entry at=20
https://bugzilla.kernel.org/show_bug.cgi?id=3D16406
for your bug report, please add your address to the CC list in there, thank=
s!
=2D-=20
Maciej Rutecki
http://www.maciek.unixy.pl
^ permalink raw reply
* Re: cpm_uart_console_write() stuck in waiting for transmitter fifo ready
From: Shawn Jin @ 2010-07-17 0:30 UTC (permalink / raw)
To: Scott Wood; +Cc: ppcdev
In-Reply-To: <20100716131333.0c99a627@schlenkerla.am.freescale.net>
>> > My RCCR=3D0x1, meaning the first 512B is for microcode. So the data an=
d
>> > the TxBD should really be starting at 0xfa202200? Then my muram data's
>> > reg should be <0x200 ?>? What size shall I specify?
>>
>> After the muram data's reg is changed to <0x200 0x1a00>, the cpm_uart
>> driver works properly and the kernel messages are printed on the
>> serial port.
>
> The muram node is supposed to show the portions of DPRAM that are
> usable by the OS. =A0If some portion has been taken up by microcode (or
> anything else not under the OS's control) before the OS has started,
> then it must be excluded from the muram node.
It would be nicer that the initialization code could query the RCCR
value and adjust the base address. It took me quite a while to
understand the design. Without your help it could take much much
longer. So thanks a lot for your help. My project hasn't been done
yet, so I may bother you again. :)
Thanks again,
-Shawn.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Jamie Lokier @ 2010-07-16 23:49 UTC (permalink / raw)
To: Daniel Walker
Cc: linuxppc-dev, linux-kbuild, Tony Lindgren, Nicolas Pitre,
linux-kernel, linux-arm-kernel, Uwe Kleine-König,
Linus Torvalds, Russell King
In-Reply-To: <1279124563.21162.14.camel@c-dwalke-linux.qualcomm.com>
Daniel Walker wrote:
> > But all the rest is arbitrary and could be part of common shared
> > profiles or the like in defconfig format.
>
> I'm sure most people will want to have a config isolated to their
> specific device. That to me seems reasonable because everyone wants the
> smallest possible kernel they can get for their given device.
Indeed, but people who want the smallest possible kernel for their
specific device _in a particular use context_ tend to want:
- To disable support for parts of the device they aren't using.
For example, an SoC with integrated ethernet that isn't actually
wired up on their board, or where they're using an external ethernet
chip instead for some reason.
- To choose what's modular and what isn't, even for integrated
parts. For example to control the bootup sequence, they might
want to delay integrated USB and IDE initialisation, which is done by
making those modular and loading them after bringing up a splash
screen earlier in the boot scripts.
So there is still a need to be able to override the drivers and
settings, but it's still incredibly useful to have defaults which
describe the SoC or board accurately.
-- Jamie
^ permalink raw reply
* Re: hi, i have two flashs, but my kernel can only find one , how can i write the dts?
From: Grant Likely @ 2010-07-16 21:46 UTC (permalink / raw)
To: hacklu; +Cc: linuxppc-dev
In-Reply-To: <201007161634429622975@gmail.com>
T24gRnJpLCBKdWwgMTYsIDIwMTAgYXQgMjozNCBBTSwgaGFja2x1IDxlbWJlZHdheS50ZXN0QGdt
YWlsLmNvbT4gd3JvdGU6Cj4gdGhpcyBpcyBteSBkdHMgZmlsZToKPiBmbGFzaEAwLDCgewo+IKCg
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoCNhZGRyZXNzLWNlbGxzoD2gPDE+Owo+IKCgoKCgoKCgoKCg
oKCgoKCgoKCgoKCgoCNzaXplLWNlbGxzoD2gPDE+Owo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oGNvbXBhdGlibGWgPaAiY2ZpLWZsYXNoIjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBwcm9i
ZS10eXBloD2gIkNGSSI7Cj4goKCgoKCgoKCgoKCgoKCgoKCgIKCgoKCgcmVnoD2gPDCgMKAxMDAw
MDAwPjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKBiYW5rLXdpZHRooD2gPDI+Owo+IKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoGRldmljZS13aWR0aKA9oDwxPjsKPiCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKBocmN3QDCgewo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgbGFiZWyg
PaAiaHJjdyI7Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKByZWegPaA8MKA0MDAw
MD47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgfTsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCg
oKBqZmZzQDQwMDAwoHsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoGxhYmVsoD2g
ImpmZnMiOwo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgcmVnoD2gPDQwMDAwoDIw
MDAwMD47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgfTsKPiCgoKCgoKCgoKCgoKCgoKCgoKCg
oKCgoKBqZmZzMkAyNDAwMDCgewo+IKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgbGFi
ZWygPaAidWltYWdlIjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoHJlZ6A9oDwy
NDAwMDCgZDgwMDAwPjsKPiCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgoKB9Owo+IKCgoKCgoKCgoKCg
oKB9Owo+IGZsYXNoQDEsMKB7Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgI2FkZHJlc3MtY2Vs
bHOgPaA8MT47Cj4goKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgI3NpemUtY2VsbHOgPaA8MT47Cj4g
oKCgoKCgoKCgoKCgoKCgoKCgoKCgoKCgY29tcGF0aWJsZaA9oCJjZmktZmxhc2giOwo+IKCgoKCg
oKCgoKCgoKCgoKCgoKCgoKCgoHByb2JlLXR5cGWgPaAiQ0ZJIjsKPiCgoKCgoKCgoKCgoKCgoKCg
oKCgoKCgoKByZWegPaA8MTAwMDAwMKAwoDEwMDAwMDA+OwoKVGhpcyBsb29rcyB3cm9uZy4gIElm
IHlvdSdyZSBzZWNvbmQgZmxhc2ggaXMgb24gY2hpcCBzZWxlY3QgMSBhcyB0aGUKbm9kZSBuYW1l
IHN1Z2dlc3RzLCB0aGVuIHRoaXMgc2hvdWxkIGJlIChmaXJzdCBjZWxsIGlzIENTIywgc2Vjb25k
IGlzCm9mZnNldCwgYW5kIHRoaXJkIGlzIHNpemUuICBBbG9zIHlvdSdyZSBtaXNzaW5nIHRoZSAw
eCBwcmVmaXgpOgoKcmVnID0gPDEgMCAweDEwMDAwMDA+OwoKSWYgeW91ciBzZWNvbmQgZmxhc2gg
aXMgb24gY2hpcCBzZWxlY3QgMCB3aXRoIHRoZSBmaXJzdCBmbGFzaCwgYnV0Cm9mZnNldCBieSAw
eDEwMDAwMDAsIHRoZW4gcmVnIHNob3VsZCBiZToKCnJlZyA9IDwwIDB4MTAwMDAwMCAweDEwMDAw
MDA+OwoKYW5kIHRoZSBuYW1lIHNob3VsZCBiZToKCmZsYXNoQDAsMTAwMDAwMCB7IC4uLiB9OwoK
Zy4KCi0tIApHcmFudCBMaWtlbHksIEIuU2MuLCBQLkVuZy4KU2VjcmV0IExhYiBUZWNobm9sb2dp
ZXMgTHRkLgo=
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: David Miller @ 2010-07-16 20:45 UTC (permalink / raw)
To: jwboyer
Cc: randy.dunlap, linuxppc-dev, linux, paulus, john.linn, davidb,
ladis, solomon, vamos-dev, vapier, florian, Artem.Bityutskiy,
qy03fugy, nico, netdev, linux-kernel, miltonm, jkosina, joe,
linux-mtd, dwmw2
In-Reply-To: <20100716142055.GA11736@zod.rchland.ibm.com>
From: Josh Boyer <jwboyer@linux.vnet.ibm.com>
Date: Fri, 16 Jul 2010 10:20:55 -0400
> On Fri, Jul 16, 2010 at 02:29:02PM +0200, Christian Dietrich wrote:
>>The config options for REDWOOD_[456] were commented out in the powerpc
>>Kconfig. The ifdefs referencing this options therefore are dead and all
>>references to this can be removed (Also dependencies in other KConfig
>>files).
>>
>>Signed-off-by: Christian Dietrich <qy03fugy@stud.informatik.uni-erlangen.de>
>>Signed-off-by: Christoph Egger <siccegge@cs.fau.de>
>
> This seems fine with me.
>
> The only question is which tree it coms through. I'm happy to take it
> in via mine if the netdev and MTD people are fine with that. Otherwise,
> my ack is below.
>
> Acked-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
Please take it:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Catalin Marinas @ 2010-07-16 20:44 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml, linuxppc-dev,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <AANLkTimKpmZaGRYfYs8HGDz-_VG8ePXN6bZvl2-YcNdR@mail.gmail.com>
On Fri, 2010-07-16 at 21:17 +0100, Grant Likely wrote:
> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> >
> >> > DOH.
> >>
> >> Well, it's possible that the correct approach is a mixture.
> >>
> >> Automatically do the trivial cases (recursive selects, dependencies
> >> that are simple or of the form "x && y" etc), and warn about the cases
> >> that aren't trivial (where "not trivial" may not necessarily be about
> >> fundamentally ambiguous ones, but just "complex enough that I won't
> >> even try").
> >
> > There is still a risk with this approach when the Kconfig isn't entirely
> > correct. For example, on ARM we have (I pushed a patch already):
> >
> > config CPU_32v6K
> > depends on CPU_V6
> >
> > config CPU_V7
> > select CPU_32v6K
> >
> > In this simple approach, we end up selecting CPU_V6 when we only need
> > CPU_V7. There other places like this in the kernel.
> >
> > Of course, kbuild could still warn but if people rely on this feature to
> > select options automatically I suspect they would ignore the warnings.
>
> In my first patch, I made Kconfig problems errors instead of warnings.
> That would prevent people from ignoring them.
My point was that if we allow kbuild to select dependencies
automatically (as per Nico's initial suggestion, followed up by Linus),
in the above situation CPU_V7 would trigger the selection of CPU_V6 and
I don't want this. If we rely on such automatic selection of the
"depends on" options, we can't make the warnings be errors.
--
Catalin
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-16 20:37 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Catalin Marinas, linuxppc-dev, lkml,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <alpine.LFD.2.00.1007161629270.10598@xanadu.home>
On Fri, Jul 16, 2010 at 2:29 PM, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Fri, 16 Jul 2010, Grant Likely wrote:
>
>> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
>> <catalin.marinas@arm.com> wrote:
>> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
>> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wr=
ote:
>> >> >
>> >> > DOH.
>> >>
>> >> Well, it's possible that the correct approach is a mixture.
>> >>
>> >> Automatically do the trivial cases (recursive selects, dependencies
>> >> that are simple or of the form "x && y" etc), and warn about the case=
s
>> >> that aren't trivial (where "not trivial" may not necessarily be about
>> >> fundamentally ambiguous ones, but just "complex enough that I won't
>> >> even try").
>> >
>> > There is still a risk with this approach when the Kconfig isn't entire=
ly
>> > correct. For example, on ARM we have (I pushed a patch already):
>> >
>> > config CPU_32v6K
>> > =A0 =A0 =A0 =A0depends on CPU_V6
>> >
>> > config CPU_V7
>> > =A0 =A0 =A0 =A0select CPU_32v6K
>> >
>> > In this simple approach, we end up selecting CPU_V6 when we only need
>> > CPU_V7. There other places like this in the kernel.
>> >
>> > Of course, kbuild could still warn but if people rely on this feature =
to
>> > select options automatically I suspect they would ignore the warnings.
>>
>> In my first patch, I made Kconfig problems errors instead of warnings.
>> =A0That would prevent people from ignoring them.
>
> ACK.
It would also flush out any current Kconfig dependency issues.
g.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Nicolas Pitre @ 2010-07-16 20:29 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Catalin Marinas, linuxppc-dev, lkml,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <AANLkTimKpmZaGRYfYs8HGDz-_VG8ePXN6bZvl2-YcNdR@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1361 bytes --]
On Fri, 16 Jul 2010, Grant Likely wrote:
> On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> >> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >> >
> >> > DOH.
> >>
> >> Well, it's possible that the correct approach is a mixture.
> >>
> >> Automatically do the trivial cases (recursive selects, dependencies
> >> that are simple or of the form "x && y" etc), and warn about the cases
> >> that aren't trivial (where "not trivial" may not necessarily be about
> >> fundamentally ambiguous ones, but just "complex enough that I won't
> >> even try").
> >
> > There is still a risk with this approach when the Kconfig isn't entirely
> > correct. For example, on ARM we have (I pushed a patch already):
> >
> > config CPU_32v6K
> > depends on CPU_V6
> >
> > config CPU_V7
> > select CPU_32v6K
> >
> > In this simple approach, we end up selecting CPU_V6 when we only need
> > CPU_V7. There other places like this in the kernel.
> >
> > Of course, kbuild could still warn but if people rely on this feature to
> > select options automatically I suspect they would ignore the warnings.
>
> In my first patch, I made Kconfig problems errors instead of warnings.
> That would prevent people from ignoring them.
ACK.
Nicolas
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-16 20:17 UTC (permalink / raw)
To: Catalin Marinas
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml, linuxppc-dev,
Uwe Kleine-König, Linus Torvalds, linux-arm-kernel
In-Reply-To: <1279310976.18579.8.camel@e102109-lin.cambridge.arm.com>
On Fri, Jul 16, 2010 at 2:09 PM, Catalin Marinas
<catalin.marinas@arm.com> wrote:
> On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
>> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote=
:
>> >
>> > DOH.
>>
>> Well, it's possible that the correct approach is a mixture.
>>
>> Automatically do the trivial cases (recursive selects, dependencies
>> that are simple or of the form "x && y" etc), and warn about the cases
>> that aren't trivial (where "not trivial" may not necessarily be about
>> fundamentally ambiguous ones, but just "complex enough that I won't
>> even try").
>
> There is still a risk with this approach when the Kconfig isn't entirely
> correct. For example, on ARM we have (I pushed a patch already):
>
> config CPU_32v6K
> =A0 =A0 =A0 =A0depends on CPU_V6
>
> config CPU_V7
> =A0 =A0 =A0 =A0select CPU_32v6K
>
> In this simple approach, we end up selecting CPU_V6 when we only need
> CPU_V7. There other places like this in the kernel.
>
> Of course, kbuild could still warn but if people rely on this feature to
> select options automatically I suspect they would ignore the warnings.
In my first patch, I made Kconfig problems errors instead of warnings.
That would prevent people from ignoring them.
g.
^ permalink raw reply
* Re: [PATCH v2] edac: mpc85xx: Add support for new MPCxxx/Pxxxx EDAC controllers
From: Scott Wood @ 2010-07-16 20:12 UTC (permalink / raw)
To: Anton Vorontsov
Cc: Peter Tyser, linux-kernel, Dave Jiang, linuxppc-dev,
Doug Thompson, Andrew Morton
In-Reply-To: <20100715182507.GA3482@oksana.dev.rtsoft.ru>
On Thu, 15 Jul 2010 22:25:07 +0400
Anton Vorontsov <avorontsov@mvista.com> wrote:
> Simply add proper IDs into the device table.
>
> Signed-off-by: Anton Vorontsov <avorontsov@mvista.com>
> ---
>
> It appears that the driver has two device ID tables. :-)
> So, my previous attempt enabled only half of the functionality.
>
> Andrew,
>
> Can you please replace
>
> edac-mpc85xx-add-support-for-mpc8569-edac-controllers.patch
>
> with this patch? It also adds some more IDs for the newer chips.
>
> Thanks!
>
> drivers/edac/mpc85xx_edac.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/edac/mpc85xx_edac.c b/drivers/edac/mpc85xx_edac.c
> index 52ca09b..3820879 100644
> --- a/drivers/edac/mpc85xx_edac.c
> +++ b/drivers/edac/mpc85xx_edac.c
> @@ -646,8 +646,12 @@ static struct of_device_id mpc85xx_l2_err_of_match[] = {
> { .compatible = "fsl,mpc8555-l2-cache-controller", },
> { .compatible = "fsl,mpc8560-l2-cache-controller", },
> { .compatible = "fsl,mpc8568-l2-cache-controller", },
> + { .compatible = "fsl,mpc8569-l2-cache-controller", },
> { .compatible = "fsl,mpc8572-l2-cache-controller", },
> + { .compatible = "fsl,p1020-l2-cache-controller", },
> + { .compatible = "fsl,p1021-l2-cache-controller", },
> { .compatible = "fsl,p2020-l2-cache-controller", },
> + { .compatible = "fsl,p4080-l2-cache-controller", },
L2 on the p4080 is quite different from those other chips. It's part
of the core, controlled by SPRs.
-Scott
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Arnd Bergmann @ 2010-07-16 20:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
Nicolas Pitre, Tony Lindgren, Catalin Marinas, linux-kbuild, lkml,
Uwe Kleine-König, linuxppc-dev, linux-arm-kernel
In-Reply-To: <AANLkTikqHnyFVJF0Pzwy2xtKeNSXYAPFdPkAkLiXK8Ls@mail.gmail.com>
On Friday 16 July 2010 20:46:17 Linus Torvalds wrote:
> Maybe a full "solver" is unnecessary, for example, but just a simple
> "automatically enable the direct dependencies and scream when it's not
> simple any more" would take care of 99% of the common cases, and then
> warn when it needs some manual help.
I think the recursion should also be limited to cases where the
dependency is a valid selectable option, i.e. not for
# this architecture does not support MMIO
config HAS_IOMEM
def_bool 'n'
config PCI
bool "PCI Device drivers"
depends on HAS_IOMEM
config FOO
tristate "Some device driver"
depends on PCI
In this case, it would be straightforward for the solver to enable PCI
for when something selects CONFIG_FOO, but it should print a warning
if this is attempted while HAS_IOMEM is unconditionally disabled,
since that puts it into the "not simple" category.
Arnd
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Catalin Marinas @ 2010-07-16 20:09 UTC (permalink / raw)
To: Linus Torvalds
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Nicolas Pitre, lkml,
Uwe Kleine-König, linuxppc-dev, linux-arm-kernel
In-Reply-To: <AANLkTikqHnyFVJF0Pzwy2xtKeNSXYAPFdPkAkLiXK8Ls@mail.gmail.com>
On Fri, 2010-07-16 at 19:46 +0100, Linus Torvalds wrote:
> On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
> >
> > DOH.
>
> Well, it's possible that the correct approach is a mixture.
>
> Automatically do the trivial cases (recursive selects, dependencies
> that are simple or of the form "x && y" etc), and warn about the cases
> that aren't trivial (where "not trivial" may not necessarily be about
> fundamentally ambiguous ones, but just "complex enough that I won't
> even try").
There is still a risk with this approach when the Kconfig isn't entirely
correct. For example, on ARM we have (I pushed a patch already):
config CPU_32v6K
depends on CPU_V6
config CPU_V7
select CPU_32v6K
In this simple approach, we end up selecting CPU_V6 when we only need
CPU_V7. There other places like this in the kernel.
Of course, kbuild could still warn but if people rely on this feature to
select options automatically I suspect they would ignore the warnings.
--
Catalin
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Arnd Bergmann @ 2010-07-16 20:01 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Rothwell, Daniel Walker, linux-kbuild, Tony Lindgren,
Catalin Marinas, Nicolas Pitre, linux-kernel, Linus Torvalds,
Russell King, Uwe Kleine-König, linuxppc-dev,
linux-arm-kernel
In-Reply-To: <AANLkTinyFBTKYaryFANYheTuP-biJUws2x01NRNZPb4M@mail.gmail.com>
On Friday 16 July 2010 19:57:55 Grant Likely wrote:
> On Fri, Jul 16, 2010 at 10:03 AM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Wed, 2010-07-14 at 00:04 +0100, Grant Likely wrote:
>
> sfr and I were talking about your patch the other day. Just warning
> on incomplete dependencies is enough to make it actually workable for
> me (without my ugly post-processing step). I was very happy to hear
> that it is in linux-next.
>
> Last missing piece is being able to do "select FOO = n", which Stephen
> is currently working on.
Are there a lot of symbols for which this is needed? If there is only
a handful, you could work around this by selectively adding
config FOO
bool "foo"
default !FOO_DISABLE
config FOO_DISABLE
def_bool "n"
Arnd
^ permalink raw reply
* Re: Badness with the kernel version 2.6.35-rc1-git1 running on P6 box
From: David Rientjes @ 2010-07-16 19:19 UTC (permalink / raw)
To: Dave Hansen
Cc: sachinp, Eric Dumazet, LKML, linuxppc-dev, Jan-Bernd Themann,
netdev, David Miller, divya
In-Reply-To: <1279301731.9207.239.camel@nimitz>
On Fri, 16 Jul 2010, Dave Hansen wrote:
> > > SLUB: Unable to allocate memory on node -1 (gfp=0x20)
> > > cache: kmalloc-16384, object size: 16384, buffer size: 16384,
> > default order: 2, min order: 0
> > > node 0: slabs: 28, objs: 292, free: 0
> > > ip: page allocation failure. order:0, mode:0x8020
> > > Call Trace:
> > > [c000000006a0eb40] [c000000000011c30] .show_stack+0x6c/0x16c (unreliable)
> > > [c000000006a0ebf0] [c00000000012129c] .__alloc_pages_nodemask+0x6a0/0x75c
> > > [c000000006a0ed70] [c0000000001527cc] .alloc_pages_current+0xc4/0x104
> > > [c000000006a0ee10] [c00000000011fca4] .__get_free_pages+0x18/0x90
> > > [c000000006a0ee90] [c0000000004f7058] .ehea_get_stats+0x4c/0x1bc
> > > [c000000006a0ef30] [c0000000005a0a04] .dev_get_stats+0x38/0x64
> > > [c000000006a0efc0] [c0000000005b456c] .rtnl_fill_ifinfo+0x35c/0x85c
> > > [c000000006a0f150] [c0000000005b5920] .rtmsg_ifinfo+0x164/0x204
> > > [c000000006a0f210] [c0000000005a6d6c] .dev_change_flags+0x4c/0x7c
> > > [c000000006a0f2a0] [c0000000005b50b4] .do_setlink+0x31c/0x750
> > > [c000000006a0f3b0] [c0000000005b6724] .rtnl_newlink+0x388/0x618
> > > [c000000006a0f5f0] [c0000000005b6350] .rtnetlink_rcv_msg+0x268/0x2b4
> > > [c000000006a0f6a0] [c0000000005cfdc0] .netlink_rcv_skb+0x74/0x108
> > > [c000000006a0f730] [c0000000005b60c4] .rtnetlink_rcv+0x38/0x5c
> > > [c000000006a0f7c0] [c0000000005cf8c8] .netlink_unicast+0x318/0x3f4
> > > [c000000006a0f890] [c0000000005d05b4] .netlink_sendmsg+0x2d0/0x310
> > > [c000000006a0f970] [c00000000058e1e8] .sock_sendmsg+0xd4/0x110
> > > [c000000006a0fb50] [c00000000058e514] .SyS_sendmsg+0x1f4/0x288
> > > [c000000006a0fd70] [c00000000058c2b8] .SyS_socketcall+0x214/0x280
> > > [c000000006a0fe30] [c0000000000085b4] syscall_exit+0x0/0x40
> > > Mem-Info:
> > > Node 0 DMA per-cpu:
> > > CPU 0: hi: 0, btch: 1 usd: 0
> > > CPU 1: hi: 0, btch: 1 usd: 0
> > > CPU 2: hi: 0, btch: 1 usd: 0
> > > CPU 3: hi: 0, btch: 1 usd: 0
> > >
> > > The mainline 2.6.35-rc5 worked fine.
> >
> > Maybe you were lucky with 2.6.35-rc5
> >
> > Anyway ehea should not use GFP_ATOMIC in its ehea_get_stats() method,
> > called in process context, but GFP_KERNEL.
> >
> > Another patch is needed for ehea_refill_rq_def() as well.
>
> You're right that this is abusing GFP_ATOMIC.
>
> But is, this is just a normal "GFP_ATOMIC" allocation failure? "SLUB:
> Unable to allocate memory on node -1" seems like a somewhat
> inappropriate error message for that.
>
The slub message is seperate and doesn't generate a call trace, even
though it is a (minimum) order-0 GFP_ATOMIC allocation as well. The page
allocation failure is seperate instance that is calling the page
allocator, not the slab allocator.
> It isn't immediately obvious where the -1 is coming from. Does it truly
> mean "allocate from any node" here, or is that a buglet in and of
> itself?
>
Yes, slub uses -1 to indicate that the allocation need not come from a
specific node.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-16 18:52 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Stephen Rothwell, Daniel Walker, linux-kbuild, Tony Lindgren,
Catalin Marinas, Nicolas Pitre, linux-kernel, Linus Torvalds,
Uwe Kleine-König, linuxppc-dev, linux-arm-kernel
In-Reply-To: <20100716183028.GB26854@n2100.arm.linux.org.uk>
On Fri, Jul 16, 2010 at 12:30 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Jul 16, 2010 at 02:19:31PM -0400, Nicolas Pitre wrote:
>> For example, if I want CONFIG_MTD_CMDLINE_PARTS=3Dy, the system may be
>> smart enough to notice and automatically enable CONFIG_MTD and
>> CONFIG_MTD_PARTITIONS without having to carry those in the defconfig.
>
> How do you sort out something like this:
>
> config FOO
> =A0 =A0 =A0 =A0bool "Foo"
> =A0 =A0 =A0 =A0depends on (A && B) || C
>
> Do you enable A and B, A, B and C or just C?
>
> Bear in mind that A could be 'X86', 'M68K' or any other arch specific
> symbol.
>
> I prefer the warning method because it prompts you to investigate what's
> changed and sort out the problem by ensuring that the appropriate symbols
> are also selected. =A0The automatic selection of dependencies method carr=
ies
> the risk that it'll do the wrong thing with the above scenario.
Good point.
g.
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Linus Torvalds @ 2010-07-16 18:46 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Stephen Rothwell, Daniel Walker, Russell King - ARM Linux,
linux-kbuild, Tony Lindgren, Catalin Marinas,
Uwe Kleine-König, lkml, linuxppc-dev, linux-arm-kernel
In-Reply-To: <alpine.LFD.2.00.1007161439240.10598@xanadu.home>
On Fri, Jul 16, 2010 at 11:40 AM, Nicolas Pitre <nico@fluxnic.net> wrote:
>
> DOH.
Well, it's possible that the correct approach is a mixture.
Automatically do the trivial cases (recursive selects, dependencies
that are simple or of the form "x && y" etc), and warn about the cases
that aren't trivial (where "not trivial" may not necessarily be about
fundamentally ambiguous ones, but just "complex enough that I won't
even try").
Maybe a full "solver" is unnecessary, for example, but just a simple
"automatically enable the direct dependencies and scream when it's not
simple any more" would take care of 99% of the common cases, and then
warn when it needs some manual help.
So it's not a strict "one or the other" issue. The solution could be
"some of both".
Linus
^ permalink raw reply
* Re: [PATCH 1/5] v2 Split the memory_block structure
From: Dave Hansen @ 2010-07-16 18:45 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: linux-mm, linux-kernel, KAMEZAWA Hiroyuki, linuxppc-dev
In-Reply-To: <4C40A3BC.3060504@austin.ibm.com>
On Fri, 2010-07-16 at 13:23 -0500, Nathan Fontenot wrote:
> >> - if (mem->state != from_state_req) {
> >> - ret = -EINVAL;
> >> - goto out;
> >> + list_for_each_entry(mbs, &mem->sections, next) {
> >> + if (mbs->state != from_state_req)
> >> + continue;
> >> +
> >> + ret = memory_block_action(mbs, to_state);
> >> + if (ret)
> >> + break;
> >> + }
> >> +
> >> + if (ret) {
> >> + list_for_each_entry(mbs, &mem->sections, next) {
> >> + if (mbs->state == from_state_req)
> >> + continue;
> >> +
> >> + if (memory_block_action(mbs, to_state))
> >> + printk(KERN_ERR "Could not re-enable memory "
> >> + "section %lx\n", mbs->phys_index);
> >> + }
> >> }
> >
> > Please just use a goto here. It's nicer looking, and much more in line
> > with what's there already.
>
> Not sure if I follow on where you want the goto. If you mean after the
> if (memory_block_action())... I purposely did not have a goto here.
> Since this is in the recovery path I wanted to make sure we tried to return
> every memory section to the original state.
Looking at it a little closer, I see what you're doing now.
First of all, should memory_block_action() get a new name since it isn
not taking 'memory_block_section's?
The thing I would have liked to see is to have that error handling block
out of the way a bit. But, the function is small, and there's not _too_
much code in there, so what you have is probably the best way to do it.
Minor nit: Please pull the memory_block_action() out of the if() and do
the:
> >> + ret = memory_block_action(mbs, to_state);
> >> + if (ret)
> >> + break;
thing like above. It makes it much more obvious that the loop is
related to the top one. I was thinking if it made sense to have a
helper function to go through and do that list walk, so you could do:
ret = set_all_states(mem->sections, to_state);
if (ret)
set_all_states(mem->sections, old_state);
But I think you'd need to pass in a bit more information, so it probably
isn't worth doing that, either.
-- Dave
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Nicolas Pitre @ 2010-07-16 18:40 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Stephen Rothwell, Daniel Walker, linux-kbuild, Tony Lindgren,
Catalin Marinas, Uwe Kleine-König, lkml, Linus Torvalds,
linuxppc-dev, linux-arm-kernel
In-Reply-To: <20100716183028.GB26854@n2100.arm.linux.org.uk>
On Fri, 16 Jul 2010, Russell King - ARM Linux wrote:
> On Fri, Jul 16, 2010 at 02:19:31PM -0400, Nicolas Pitre wrote:
> > For example, if I want CONFIG_MTD_CMDLINE_PARTS=y, the system may be
> > smart enough to notice and automatically enable CONFIG_MTD and
> > CONFIG_MTD_PARTITIONS without having to carry those in the defconfig.
>
> How do you sort out something like this:
>
> config FOO
> bool "Foo"
> depends on (A && B) || C
DOH.
Nicolas
^ permalink raw reply
* Re: [PATCH 1/5] v2 Split the memory_block structure
From: Dave Hansen @ 2010-07-16 18:33 UTC (permalink / raw)
To: Nathan Fontenot; +Cc: linux-mm, linux-kernel, KAMEZAWA Hiroyuki, linuxppc-dev
In-Reply-To: <4C40A3BC.3060504@austin.ibm.com>
On Fri, 2010-07-16 at 13:23 -0500, Nathan Fontenot wrote:
> > If the memory_block's state was inferred to be the same as each
> > memory_block_section, couldn't we just keep a start and end phys_index
> > in the memory_block, and get away from having memory_block_sections at
> > all?
>
> Oooohhh... I like. Looking at the code it appears this is possible. I'll
> try this out and include it in the next version of the patch.
>
> Do you think we need to add an additional file to each memory block directory
> to indicate the number of memory sections in the memory block that are actually
> present?
I think it's easiest to just say that each 'memory_block' can only hold
contiguous 'memory_block_sections', and we give either the start/end or
start/length pairs. It gets a lot more complicated if we have to deal
with lots of holes.
I can just see the hardware designers reading this thread, with their
Dr. Evil laughs trying to come up with a reason to give us a couple of
terabytes of RAM with only every-other 16MB area populated. :)
-- Dave
^ permalink raw reply
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