* 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 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: 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 V4] powerpc/prom: Export device tree physical address via proc
From: Benjamin Herrenschmidt @ 2010-07-18 23:41 UTC (permalink / raw)
To: Grant Likely; +Cc: Matthew McClintock, Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <AANLkTimiM1Ky7aIYoQ3efhivypV90LFG-BpueuaPaiPf@mail.gmail.com>
On Thu, 2010-07-15 at 00:21 -0600, Grant Likely wrote:
> On Wed, Jul 14, 2010 at 9:18 AM, Matthew McClintock <msm@freescale.com> wrote:
> > To build a proper flat device tree for kexec we need to know which
> > memreserve region was used for the device tree for the currently
> > running kernel, so we can remove it and replace it with the new
> > memreserve for the kexec'ed kernel
> >
> > Signed-off-by: Matthew McClintock <msm@freescale.com>
>
> Hi Matthew.
>
> I don't understand. Why does userspace need to know about the old
> memreserve sections? Doesn't kexec tear down all of the old
> allocations anyway? How are they relevant for constructing the dtb
> for the kexec kernel? I'll need a lot more details before I consider
> merging this.
>
> Also, please cc: me and Ben Herrenschmidt on powerpc related device
> tree changes.
I admit to be the victim of a similar misunderstanding...
Care to explain in more details ? (with examples)
Cheers,
Ben.
> Cheers,
> g.
>
> > ---
> > V4: Fixed misspelling
> >
> > V3: Remove unneeded cast, and fixed indentation screw up
> >
> > V2: messed up changes
> >
> > arch/powerpc/kernel/prom.c | 45 ++++++++++++++++++++++++++++++++++++++++++++
> > 1 files changed, 45 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index fd9359a..ff3e240 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -32,6 +32,7 @@
> > #include <linux/debugfs.h>
> > #include <linux/irq.h>
> > #include <linux/lmb.h>
> > +#include <linux/bootmem.h>
> >
> > #include <asm/prom.h>
> > #include <asm/rtas.h>
> > @@ -911,3 +912,47 @@ static int __init export_flat_device_tree(void)
> > }
> > __initcall(export_flat_device_tree);
> > #endif
> > +
> > +#ifdef CONFIG_KEXEC
> > +static phys_addr_t flat_dt_start;
> > +static phys_addr_t flat_dt_end;
> > +
> > +static struct property flat_dt_start_prop = {
> > + .name = "linux,devicetree-start",
> > + .length = sizeof(phys_addr_t),
> > + .value = &flat_dt_start,
> > +};
> > +
> > +static struct property flat_dt_end_prop = {
> > + .name = "linux,devicetree-end",
> > + .length = sizeof(phys_addr_t),
> > + .value = &flat_dt_end,
> > +};
> > +
> > +static int __init export_flat_device_tree_phys_addr(void)
> > +{
> > + struct property *prop;
> > + struct device_node *node;
> > +
> > + node = of_find_node_by_path("/chosen");
> > + if (!node)
> > + return -ENOENT;
> > +
> > + prop = of_find_property(node, "linux,devicetree-start", NULL);
> > + if (prop)
> > + prom_remove_property(node, prop);
> > +
> > + prop = of_find_property(node, "linux,devicetree-end", NULL);
> > + if (prop)
> > + prom_remove_property(node, prop);
> > +
> > + flat_dt_start = virt_to_phys(initial_boot_params);
> > + flat_dt_end = virt_to_phys(initial_boot_params)
> > + + initial_boot_params->totalsize;
> > + prom_add_property(node, &flat_dt_start_prop);
> > + prom_add_property(node, &flat_dt_end_prop);
> > +
> > + return 0;
> > +}
> > +__initcall(export_flat_device_tree_phys_addr);
> > +#endif
> > --
> > 1.6.6.1
> >
> >
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> >
>
>
>
^ permalink raw reply
* Re: [PATCH 1/2] Remove REDWOOD_[456] config options and conditional code
From: Benjamin Herrenschmidt @ 2010-07-19 0:00 UTC (permalink / raw)
To: Christian Dietrich
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, Nicolas Pitre,
Jiri Kosina, linux-kernel, Milton Miller, netdev, Joe Perches,
linux-mtd, David Woodhouse, David S. Miller
In-Reply-To: <ca1bb25d203618c3548748f5efb6f125a96c89e0.1279282865.git.qy03fugy@stud.informatik.uni-erlangen.de>
On Fri, 2010-07-16 at 14:29 +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>
Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> arch/powerpc/platforms/40x/Kconfig | 16 -------------
> drivers/mtd/maps/Kconfig | 2 +-
> drivers/mtd/maps/redwood.c | 43 ------------------------------------
> drivers/net/Kconfig | 2 +-
> drivers/net/smc91x.h | 37 -------------------------------
> 5 files changed, 2 insertions(+), 98 deletions(-)
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index ec64264..b721764 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -71,22 +71,6 @@ config MAKALU
> help
> This option enables support for the AMCC PPC405EX board.
>
> -#config REDWOOD_5
> -# bool "Redwood-5"
> -# depends on 40x
> -# default n
> -# select STB03xxx
> -# help
> -# This option enables support for the IBM STB04 evaluation board.
> -
> -#config REDWOOD_6
> -# bool "Redwood-6"
> -# depends on 40x
> -# default n
> -# select STB03xxx
> -# help
> -# This option enables support for the IBM STBx25xx evaluation board.
> -
> #config SYCAMORE
> # bool "Sycamore"
> # depends on 40x
> diff --git a/drivers/mtd/maps/Kconfig b/drivers/mtd/maps/Kconfig
> index f22bc9f..6629d09 100644
> --- a/drivers/mtd/maps/Kconfig
> +++ b/drivers/mtd/maps/Kconfig
> @@ -321,7 +321,7 @@ config MTD_CFI_FLAGADM
>
> config MTD_REDWOOD
> tristate "CFI Flash devices mapped on IBM Redwood"
> - depends on MTD_CFI && ( REDWOOD_4 || REDWOOD_5 || REDWOOD_6 )
> + depends on MTD_CFI
> help
> This enables access routines for the flash chips on the IBM
> Redwood board. If you have one of these boards and would like to
> diff --git a/drivers/mtd/maps/redwood.c b/drivers/mtd/maps/redwood.c
> index 933c0b6..d2c9db0 100644
> --- a/drivers/mtd/maps/redwood.c
> +++ b/drivers/mtd/maps/redwood.c
> @@ -22,8 +22,6 @@
>
> #include <asm/io.h>
>
> -#if !defined (CONFIG_REDWOOD_6)
> -
> #define WINDOW_ADDR 0xffc00000
> #define WINDOW_SIZE 0x00400000
>
> @@ -69,47 +67,6 @@ static struct mtd_partition redwood_flash_partitions[] = {
> }
> };
>
> -#else /* CONFIG_REDWOOD_6 */
> -/* FIXME: the window is bigger - armin */
> -#define WINDOW_ADDR 0xff800000
> -#define WINDOW_SIZE 0x00800000
> -
> -#define RW_PART0_OF 0
> -#define RW_PART0_SZ 0x400000 /* 4 MiB data */
> -#define RW_PART1_OF RW_PART0_OF + RW_PART0_SZ
> -#define RW_PART1_SZ 0x10000 /* 64K VPD */
> -#define RW_PART2_OF RW_PART1_OF + RW_PART1_SZ
> -#define RW_PART2_SZ 0x400000 - (0x10000 + 0x20000)
> -#define RW_PART3_OF RW_PART2_OF + RW_PART2_SZ
> -#define RW_PART3_SZ 0x20000
> -
> -static struct mtd_partition redwood_flash_partitions[] = {
> - {
> - .name = "Redwood filesystem",
> - .offset = RW_PART0_OF,
> - .size = RW_PART0_SZ
> - },
> - {
> - .name = "Redwood OpenBIOS Vital Product Data",
> - .offset = RW_PART1_OF,
> - .size = RW_PART1_SZ,
> - .mask_flags = MTD_WRITEABLE /* force read-only */
> - },
> - {
> - .name = "Redwood kernel",
> - .offset = RW_PART2_OF,
> - .size = RW_PART2_SZ
> - },
> - {
> - .name = "Redwood OpenBIOS",
> - .offset = RW_PART3_OF,
> - .size = RW_PART3_SZ,
> - .mask_flags = MTD_WRITEABLE /* force read-only */
> - }
> -};
> -
> -#endif /* CONFIG_REDWOOD_6 */
> -
> struct map_info redwood_flash_map = {
> .name = "IBM Redwood",
> .size = WINDOW_SIZE,
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index ce2fcdd..313d306 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -913,7 +913,7 @@ config SMC91X
> tristate "SMC 91C9x/91C1xxx support"
> select CRC32
> select MII
> - depends on ARM || REDWOOD_5 || REDWOOD_6 || M32R || SUPERH || \
> + depends on ARM || M32R || SUPERH || \
> MIPS || BLACKFIN || MN10300 || COLDFIRE
> help
> This is a driver for SMC's 91x series of Ethernet chipsets,
> diff --git a/drivers/net/smc91x.h b/drivers/net/smc91x.h
> index 8d2772c..ee74791 100644
> --- a/drivers/net/smc91x.h
> +++ b/drivers/net/smc91x.h
> @@ -83,43 +83,6 @@ static inline void SMC_outw(u16 val, void __iomem *ioaddr, int reg)
> }
> }
>
> -#elif defined(CONFIG_REDWOOD_5) || defined(CONFIG_REDWOOD_6)
> -
> -/* We can only do 16-bit reads and writes in the static memory space. */
> -#define SMC_CAN_USE_8BIT 0
> -#define SMC_CAN_USE_16BIT 1
> -#define SMC_CAN_USE_32BIT 0
> -#define SMC_NOWAIT 1
> -
> -#define SMC_IO_SHIFT 0
> -
> -#define SMC_inw(a, r) in_be16((volatile u16 *)((a) + (r)))
> -#define SMC_outw(v, a, r) out_be16((volatile u16 *)((a) + (r)), v)
> -#define SMC_insw(a, r, p, l) \
> - do { \
> - unsigned long __port = (a) + (r); \
> - u16 *__p = (u16 *)(p); \
> - int __l = (l); \
> - insw(__port, __p, __l); \
> - while (__l > 0) { \
> - *__p = swab16(*__p); \
> - __p++; \
> - __l--; \
> - } \
> - } while (0)
> -#define SMC_outsw(a, r, p, l) \
> - do { \
> - unsigned long __port = (a) + (r); \
> - u16 *__p = (u16 *)(p); \
> - int __l = (l); \
> - while (__l > 0) { \
> - /* Believe it or not, the swab isn't needed. */ \
> - outw( /* swab16 */ (*__p++), __port); \
> - __l--; \
> - } \
> - } while (0)
> -#define SMC_IRQ_FLAGS (0)
> -
> #elif defined(CONFIG_SA1100_PLEB)
> /* We can only do 16-bit reads and writes in the static memory space. */
> #define SMC_CAN_USE_8BIT 1
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Benjamin Herrenschmidt @ 2010-07-19 0:09 UTC (permalink / raw)
To: Grant Likely; +Cc: Matthew McClintock, Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <AANLkTikIGXBNU7NQEL7riZYB-BJU3jhxauzGo6XhUF3t@mail.gmail.com>
On Thu, 2010-07-15 at 10:22 -0600, Grant Likely wrote:
> What is your starting point? Where does the device tree (and
> memreserve list) come from
> that you're passing to kexec? My first impression is that if you have
> to scrub the memreserve list, then the source being used to
> obtain the memreserves is either faulty or unsuitable to the task.
The kernel should ultimately pass the thing to userspace I reckon, with
an appropriate hook for platform code to insert/recover reserved
regions.
Ben.
^ permalink raw reply
* Re: linux-next: build failure after merge of the final tree (powerpc related)
From: Benjamin Herrenschmidt @ 2010-07-19 0:15 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Martyn Welch, linuxppc-dev, linux-next, Paul Mackerras,
linux-kernel
In-Reply-To: <20100716171920.51ce1b88.sfr@canb.auug.org.au>
On Fri, 2010-07-16 at 17:19 +1000, Stephen Rothwell wrote:
> Hi all,
>
> After merging the final tree, today's linux-next build (powerpc
> allmodconfig) failed like this:
>
> ERROR: "of_i8042_kbd_irq" [drivers/input/serio/i8042.ko] undefined!
> ERROR: "of_i8042_aux_irq" [drivers/input/serio/i8042.ko] undefined!
>
> Presumably missing EXPORT_SYMBOLs ..
Thanks. I'll fix that up.
Cheers,
Ben.
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings while Building a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Benjamin Herrenschmidt @ 2010-07-19 1:11 UTC (permalink / raw)
To: subrata
Cc: sachinp, Alexander Graf, linux-kernel, Kamalesh Babulal,
Linuxppc-dev, Paul Mackerras, Paul Mackerras, divya.vikas
In-Reply-To: <1279193743.10707.5.camel@subratamodak.linux.ibm.com>
On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
> commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
> 2.6.34-rc6. However some other bad relocation warnings generated against
> 2.6.35-rc5 on Power7/ppc64 below:
>
> MODPOST 2004 modules^M
> WARNING: 2 bad relocations^M
> c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
> c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
I think this is KVM + CONFIG_RELOCATABLE. Caused by:
.global kvmppc_trampoline_lowmem
kvmppc_trampoline_lowmem:
.long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
.global kvmppc_trampoline_enter
kvmppc_trampoline_enter:
.long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
Alex, can you turn these into 64-bit on ppc64 so the relocator
can grok them ?
Cheers,
Ben.
> Config file attached.
>
> Regards--
> Subrata
>
> On Fri, 2010-05-07 at 15:40 +1000, Paul Mackerras wrote:
> > On Wed, May 05, 2010 at 05:20:51PM +0530, Subrata Modak wrote:
> >
> > > I built 2.6.34-rc6 with the attached Fedora Config file
> > > (config-2.6.33.1-19.fc13.ppc64) on my P5 Fedora Box and got the
> > > following warning. Is the following expected ?
> > >
> > > CALL arch/powerpc/relocs_check.pl
> > > Building modules, stage 2.
> > > WARNING: 4 bad relocations
> > > c00000000007216e R_PPC64_ADDR16_HIGHEST __ksymtab+0x00000000009dcec8
> > > c000000000072172 R_PPC64_ADDR16_HIGHER __ksymtab+0x00000000009dcec8
> > > c00000000007217a R_PPC64_ADDR16_HI __ksymtab+0x00000000009dcec8
> > > c00000000007217e R_PPC64_ADDR16_LO __ksymtab+0x00000000009dcec8
> >
> > No, it's not expected. It's in iSeries code, so you could avoid it
> > just by disabling CONFIG_ISERIES (I don't think any distro still
> > supports legacy iSeries). I'll post a patch to fix the problem
> > properly.
> >
> > Paul.
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
^ permalink raw reply
* Re: Re: hi, i have two flashs, but my kernel can only find one , how cani write the dts?
From: hacklu @ 2010-07-19 3:56 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
In-Reply-To: <AANLkTimXGTg41XHChzcnn75Px_pitC8sPKOY-OlPNIe1@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 2122 bytes --]
thanks very much! it works well now~~
but i found in my system,if i add the 0x prefix it will cause a syntax error
2010-07-19
hacklu
发件人： Grant Likely
发送时间： 2010-07-17 05:46:59
收件人： hacklu
抄送： linuxppc-dev
主题： Re: hi, i have two flashs, but my kernel can only find one , how cani write the dts?
On Fri, Jul 16, 2010 at 2:34 AM, hacklu <embedway.test@gmail.com> wrote:
> this is my dts file:
> flash@0,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "cfi-flash";
> probe-type = "CFI";
> reg = <0 0 1000000>;
> bank-width = <2>;
> device-width = <1>;
> hrcw@0 {
> label = "hrcw";
> reg = <0 40000>;
> };
> jffs@40000 {
> label = "jffs";
> reg = <40000 200000>;
> };
> jffs2@240000 {
> label = "uimage";
> reg = <240000 d80000>;
> };
> };
> flash@1,0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "cfi-flash";
> probe-type = "CFI";
> reg = <1000000 0 1000000>;
This looks wrong. If you're second flash is on chip select 1 as the
node name suggests, then this should be (first cell is CS#, second is
offset, and third is size. Alos you're missing the 0x prefix):
reg = <1 0 0x1000000>;
If your second flash is on chip select 0 with the first flash, but
offset by 0x1000000, then reg should be:
reg = <0 0x1000000 0x1000000>;
and the name should be:
flash@0,1000000 { ... };
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
[-- Attachment #1.2: Type: text/html, Size: 9255 bytes --]
[-- Attachment #2: 14.gif --]
[-- Type: image/gif, Size: 1662 bytes --]
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Matthew McClintock @ 2010-07-19 4:24 UTC (permalink / raw)
To: Segher Boessenkool
Cc: Kumar Gala, Mitch Bradley, linuxppc-dev, Timur Tabi, David Gibson
In-Reply-To: <1E87551E-BD4C-4B0B-A603-908C7D9C1D5F@kernel.crashing.org>
On Jul 17, 2010, at 11:41 AM, Segher Boessenkool wrote:
>>>>> Yes. Where would we get a list of memreserve sections?
>>>>=20
>>>> 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.
>>>=20
>>> 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?
>>=20
>> 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.
>=20
> 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?
>=20
On the platform I'm working on (mpc85xx) they can be the following:
1) Boot page (aka cpu-release-addr) - always present on MP
2) Flat Device Tree - always present
3) Initrd - optional
When kexec'ing a kernel, we will provide new memory regions for the flat =
device tree and the initrd (if present). However, all others will not be =
replaced by kexec and should either be tossed or preserved. The question =
is how to decide what to do... save them all by default? Save none of =
them? If we save them all at a minimum we need to remove/replace the =
device tree and initrd regions as well. So we need a way to identify =
which regions correspond to these.
Grant and I talked and though a property that lives in the device tree =
describing a persistant-memreseve might work. And Mitch talked about an =
available memory property to go along with the current one which could =
be used to determine which ranges were invalid and need a memreserve =
for...
-Matthew
>=20
> Segher
>=20
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>=20
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Matthew McClintock @ 2010-07-19 4:28 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <1279496491.10390.1725.camel@pasglop>
On Jul 18, 2010, at 6:41 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2010-07-15 at 00:21 -0600, Grant Likely wrote:
>> On Wed, Jul 14, 2010 at 9:18 AM, Matthew McClintock =
<msm@freescale.com> wrote:
>>> To build a proper flat device tree for kexec we need to know which
>>> memreserve region was used for the device tree for the currently
>>> running kernel, so we can remove it and replace it with the new
>>> memreserve for the kexec'ed kernel
>>>=20
>>> Signed-off-by: Matthew McClintock <msm@freescale.com>
>>=20
>> Hi Matthew.
>>=20
>> I don't understand. Why does userspace need to know about the old
>> memreserve sections? Doesn't kexec tear down all of the old
>> allocations anyway? How are they relevant for constructing the dtb
>> for the kexec kernel? I'll need a lot more details before I consider
>> merging this.
>>=20
>> Also, please cc: me and Ben Herrenschmidt on powerpc related device
>> tree changes.
>=20
> I admit to be the victim of a similar misunderstanding...
>=20
> Care to explain in more details ? (with examples)
>=20
Upon first examining the details of getting kexec working on our =
platform I noticed our device tree passed from u-boot contained an =
additional memreserve section for the boot page. Subsequently, I've been =
trying to preserve the ones passed in for the kexec'ed kernel thinking =
anyone could add anything they wanted for their own particular needs and =
would quite possibly need those regions preserved across reboots.
Recently, I've discovered the boot page address is given in the device =
tree as a property. So, for our platform (mpc85xx) in particular, I'm =
back to not needing the read the old memreserve sections... I can =
completely reconstruct the appropriate memreserve regions for kexec'ing =
from the information passed in the device tree.
That isn't to say there might not be more memreserve regions that are =
not there for some valid reason. I'm not sure if we need to attempt to =
address another scenario where there are other memreserve regions. So =
this would be a good time to address this issue if anyone believes it is =
a worthwhile pursuit to have a mechanism to have memreserve regions =
persistent across kexec'ing - or any other reboot.
-Matthew
> Cheers,
> Ben.
>=20
>> Cheers,
>> g.
>>=20
>>> ---
>>> V4: Fixed misspelling
>>>=20
>>> V3: Remove unneeded cast, and fixed indentation screw up
>>>=20
>>> V2: messed up changes
>>>=20
>>> arch/powerpc/kernel/prom.c | 45 =
++++++++++++++++++++++++++++++++++++++++++++
>>> 1 files changed, 45 insertions(+), 0 deletions(-)
>>>=20
>>> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
>>> index fd9359a..ff3e240 100644
>>> --- a/arch/powerpc/kernel/prom.c
>>> +++ b/arch/powerpc/kernel/prom.c
>>> @@ -32,6 +32,7 @@
>>> #include <linux/debugfs.h>
>>> #include <linux/irq.h>
>>> #include <linux/lmb.h>
>>> +#include <linux/bootmem.h>
>>>=20
>>> #include <asm/prom.h>
>>> #include <asm/rtas.h>
>>> @@ -911,3 +912,47 @@ static int __init export_flat_device_tree(void)
>>> }
>>> __initcall(export_flat_device_tree);
>>> #endif
>>> +
>>> +#ifdef CONFIG_KEXEC
>>> +static phys_addr_t flat_dt_start;
>>> +static phys_addr_t flat_dt_end;
>>> +
>>> +static struct property flat_dt_start_prop =3D {
>>> + .name =3D "linux,devicetree-start",
>>> + .length =3D sizeof(phys_addr_t),
>>> + .value =3D &flat_dt_start,
>>> +};
>>> +
>>> +static struct property flat_dt_end_prop =3D {
>>> + .name =3D "linux,devicetree-end",
>>> + .length =3D sizeof(phys_addr_t),
>>> + .value =3D &flat_dt_end,
>>> +};
>>> +
>>> +static int __init export_flat_device_tree_phys_addr(void)
>>> +{
>>> + struct property *prop;
>>> + struct device_node *node;
>>> +
>>> + node =3D of_find_node_by_path("/chosen");
>>> + if (!node)
>>> + return -ENOENT;
>>> +
>>> + prop =3D of_find_property(node, "linux,devicetree-start", =
NULL);
>>> + if (prop)
>>> + prom_remove_property(node, prop);
>>> +
>>> + prop =3D of_find_property(node, "linux,devicetree-end", =
NULL);
>>> + if (prop)
>>> + prom_remove_property(node, prop);
>>> +
>>> + flat_dt_start =3D virt_to_phys(initial_boot_params);
>>> + flat_dt_end =3D virt_to_phys(initial_boot_params)
>>> + + initial_boot_params->totalsize;
>>> + prom_add_property(node, &flat_dt_start_prop);
>>> + prom_add_property(node, &flat_dt_end_prop);
>>> +
>>> + return 0;
>>> +}
>>> +__initcall(export_flat_device_tree_phys_addr);
>>> +#endif
>>> --
>>> 1.6.6.1
>>>=20
>>>=20
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>>=20
>>=20
>>=20
>>=20
>=20
>=20
>=20
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Grant Likely @ 2010-07-19 4:32 UTC (permalink / raw)
To: Matthew McClintock; +Cc: Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <8388D248-51F3-4F36-BBE0-CDE9E278E65E@freescale.com>
On Sun, Jul 18, 2010 at 10:28 PM, Matthew McClintock <msm@freescale.com> wr=
ote:
> On Jul 18, 2010, at 6:41 PM, Benjamin Herrenschmidt wrote:
>> On Thu, 2010-07-15 at 00:21 -0600, Grant Likely wrote:
>>> On Wed, Jul 14, 2010 at 9:18 AM, Matthew McClintock <msm@freescale.com>=
wrote:
>>>> To build a proper flat device tree for kexec we need to know which
>>>> memreserve region was used for the device tree for the currently
>>>> running kernel, so we can remove it and replace it with the new
>>>> memreserve for the kexec'ed kernel
>>>>
>>>> Signed-off-by: Matthew McClintock <msm@freescale.com>
>>>
>>> Hi Matthew.
>>>
>>> I don't understand. =A0Why does userspace need to know about the old
>>> memreserve sections? =A0Doesn't kexec tear down all of the old
>>> allocations anyway? =A0How are they relevant for constructing the dtb
>>> for the kexec kernel? =A0I'll need a lot more details before I consider
>>> merging this.
>>>
>>> Also, please cc: me and Ben Herrenschmidt on powerpc related device
>>> tree changes.
>>
>> I admit to be the victim of a similar misunderstanding...
>>
>> Care to explain in more details ? (with examples)
>>
>
> Upon first examining the details of getting kexec working on our platform=
I noticed our device tree passed from u-boot contained an additional memre=
serve section for the boot page. Subsequently, I've been trying to preserve=
the ones passed in for the kexec'ed kernel thinking anyone could add anyth=
ing they wanted for their own particular needs and would quite possibly nee=
d those regions preserved across reboots.
>
> Recently, I've discovered the boot page address is given in the device tr=
ee as a property. So, for our platform (mpc85xx) in particular, I'm back to=
not needing the read the old memreserve sections... I can completely recon=
struct the appropriate memreserve regions for kexec'ing from the informatio=
n passed in the device tree.
>
> That isn't to say there might not be more memreserve regions that are not=
there for some valid reason. I'm not sure if we need to attempt to address=
another scenario where there are other memreserve regions. So this would b=
e a good time to address this issue if anyone believes it is a worthwhile p=
ursuit to have a mechanism to have memreserve regions persistent across kex=
ec'ing - or any other reboot.
No, we haven't needed anything to date, so no sense trying to design a
solution for a theoretical need. Leave it be for now.
g.
^ permalink raw reply
* Re: Re: hi, i have two flashs, but my kernel can only find one , how cani write the dts?
From: Grant Likely @ 2010-07-19 4:33 UTC (permalink / raw)
To: hacklu; +Cc: linuxppc-dev
In-Reply-To: <201007191156288194097@gmail.com>
On Sun, Jul 18, 2010 at 9:56 PM, hacklu <embedway.test@gmail.com> wrote:
>
> thanks very much! it works well now~~
> but i found in my system,if i add the 0x prefix it will cause=A0a syntax =
error
Ah, your dts file is in the old version 0 format. You must be on an
older kernel, or at least haven't updated your dts file to the new
format after upgrading.
g.
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Matthew McClintock @ 2010-07-19 4:34 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <1279498153.10390.1764.camel@pasglop>
On Jul 18, 2010, at 7:09 PM, Benjamin Herrenschmidt wrote:
> On Thu, 2010-07-15 at 10:22 -0600, Grant Likely wrote:
>> What is your starting point? Where does the device tree (and
>> memreserve list) come from
>> that you're passing to kexec? My first impression is that if you =
have
>> to scrub the memreserve list, then the source being used to
>> obtain the memreserves is either faulty or unsuitable to the task.
>=20
> The kernel should ultimately pass the thing to userspace I reckon, =
with
> an appropriate hook for platform code to insert/recover reserved
> regions.
>=20
Or possibly in the device tree itself? As I mentioned in my previous =
email - my particular case can already be handled by the information =
passed in the device tree (as I recently discovered), is this something =
we would want to make generic for the device tree or add platform code =
to expose these memreserve regions?
-Matthew
^ permalink raw reply
* Re: [RFC PATCH] Kconfig: Enable Kconfig fragments to be used for defconfig
From: Grant Likely @ 2010-07-19 5:20 UTC (permalink / raw)
To: Jamie Lokier
Cc: Daniel Walker, linux-kbuild, Tony Lindgren, Nicolas Pitre,
linux-kernel, linuxppc-dev, linux-arm-kernel,
Uwe Kleine-König, Linus Torvalds, Russell King
In-Reply-To: <20100716234918.GA31060@shareable.org>
On Fri, Jul 16, 2010 at 5:49 PM, Jamie Lokier <jamie@shareable.org> wrote:
> 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.
Just to be clear (specifically for me as a maintainer) the purpose of
defconfigs is not to provide the best optimized kernel configuration
for each given board. defconfigs are useful as a reasonable working
starting point, and to provide build coverage testing.
> Indeed, but people who want the smallest possible kernel for their
> specific device _in a particular use context_ tend to want:
>
> =A0- To disable support for parts of the device they aren't using.
> =A0 =A0For example, an SoC with integrated ethernet that isn't actually
> =A0 =A0wired up on their board, or where they're using an external ethern=
et
> =A0 =A0chip instead for some reason.
>
> =A0- To choose what's modular and what isn't, even for integrated
> =A0 =A0parts. =A0For example to control the bootup sequence, they might
> =A0 =A0want to delay integrated USB and IDE initialisation, which is done=
by
> =A0 =A0making those modular and loading them after bringing up a splash
> =A0 =A0screen 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.
Yes. The defconfig is only a starting point. Maintaining the actual
config for the shipped kernel is the job of the distribution vendor
and I have zero interest in maintaining those configurations in the
kernel tree.
g.
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings while Building a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Alexander Graf @ 2010-07-19 7:36 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: sachinp, linux-kernel, Kamalesh Babulal, Linuxppc-dev,
Paul Mackerras, Paul Mackerras, subrata, divya.vikas
In-Reply-To: <1279501889.10390.1834.camel@pasglop>
On 19.07.2010, at 03:11, Benjamin Herrenschmidt wrote:
> On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
>> commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem =
for
>> 2.6.34-rc6. However some other bad relocation warnings generated =
against
>> 2.6.35-rc5 on Power7/ppc64 below:
>>=20
>> MODPOST 2004 modules^M
>> WARNING: 2 bad relocations^M
>> c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
>> c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
>=20
> I think this is KVM + CONFIG_RELOCATABLE. Caused by:
>=20
> .global kvmppc_trampoline_lowmem
> kvmppc_trampoline_lowmem:
> .long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
>=20
> .global kvmppc_trampoline_enter
> kvmppc_trampoline_enter:
> .long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
>=20
> Alex, can you turn these into 64-bit on ppc64 so the relocator
> can grok them ?
If I turn them into 64-bit, will the values be > RMA? In that case =
things would break anyways. How does relocation work on PPC? Are the =
first few megs copied over to low memory? Would I have to mask anything =
in the above code to make sure I use the real values?
Alex
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings while Building a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Subrata Modak @ 2010-07-19 8:56 UTC (permalink / raw)
To: Michael Neuling
Cc: sachinp, linux-kernel, Kamalesh Babulal, Linuxppc-dev,
Paul Mackerras, Paul Mackerras, divya.vikas
In-Reply-To: <4635.1279245899@neuling.org>
On Fri, 2010-07-16 at 12:04 +1000, Michael Neuling wrote:
> > commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
> > 2.6.34-rc6. However some other bad relocation warnings generated against
> > 2.6.35-rc5 on Power7/ppc64 below:
> >
> > MODPOST 2004 modules^M
> > WARNING: 2 bad relocations^M
> > c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
> > c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
>
> I can't replicate this with your config with gcc 4.4.4 and binutils
> 2.20. What are you using?
Well, i also have the following GCC (from Fedora 13 Rawhide)
# gcc --version
gcc (GCC) 4.4.4 20100611 (Red Hat 4.4.4-8)
Copyright (C) 2010 Free Software Foundation, Inc.
and binutils-2.20.51.0.2-22.fc13.ppc,
Regards--
Subrata
>
> Mikey
>
> >
> > Config file attached.
> >
> > Regards--
> > Subrata
> >
> > On Fri, 2010-05-07 at 15:40 +1000, Paul Mackerras wrote:
> > > On Wed, May 05, 2010 at 05:20:51PM +0530, Subrata Modak wrote:
> > >
> > > > I built 2.6.34-rc6 with the attached Fedora Config file
> > > > (config-2.6.33.1-19.fc13.ppc64) on my P5 Fedora Box and got the
> > > > following warning. Is the following expected ?
> > > >
> > > > CALL arch/powerpc/relocs_check.pl
> > > > Building modules, stage 2.
> > > > WARNING: 4 bad relocations
> > > > c00000000007216e R_PPC64_ADDR16_HIGHEST __ksymtab+0x00000000009dcec8
> > > > c000000000072172 R_PPC64_ADDR16_HIGHER __ksymtab+0x00000000009dcec8
> > > > c00000000007217a R_PPC64_ADDR16_HI __ksymtab+0x00000000009dcec8
> > > > c00000000007217e R_PPC64_ADDR16_LO __ksymtab+0x00000000009dcec8
> > >
> > > No, it's not expected. It's in iSeries code, so you could avoid it
> > > just by disabling CONFIG_ISERIES (I don't think any distro still
> > > supports legacy iSeries). I'll post a patch to fix the problem
> > > properly.
> > >
> > > Paul.
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings whileBuilding a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Milton Miller @ 2010-07-19 11:23 UTC (permalink / raw)
To: Alexander Graf; +Cc: Subrata Modak, linuxppc-dev, linux-kernel
In-Reply-To: <8C4BCF61-7F5D-4C80-A311-93CF8521FBF5@suse.de>
On Mon Jul 19 2010 at about 03:36:51 EST, Alexander Graf wrote:
> On 19.07.2010, at 03:11, Benjamin Herrenschmidt wrote:
>
> > On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
> > > commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
> > > 2.6.34-rc6. However some other bad relocation warnings generated against
> > > 2.6.35-rc5 on Power7/ppc64 below:
> > >
> > > MODPOST 2004 modules^M
> > > WARNING: 2 bad relocations^M
> > > c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
> > > c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
> >
> > I think this is KVM + CONFIG_RELOCATABLE. Caused by:
> >
> > .global kvmppc_trampoline_lowmem
> > kvmppc_trampoline_lowmem:
> > .long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
> >
> > .global kvmppc_trampoline_enter
> > kvmppc_trampoline_enter:
> > .long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
> >
> > Alex, can you turn these into 64-bit on ppc64 so the relocator
> > can grok them ?
>
> If I turn them into 64-bit, will the values be > RMA? In that case
> things would break anyways. How does relocation work on PPC? Are the
> first few megs copied over to low memory? Would I have to mask anything
> in the above code to make sure I use the real values?
>
> Alex
>
You can still do the subtraction, but you have to allocate 64 bits for
storage. Relocatable ppc64 kernels work by adjusting PPC64_RELOC_RELATIVE
entries during early boot (reloc in reloc_64.S called from head_64.S).
The code purposely only supports 64 bit relative addressing.
milton
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings whileBuilding a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Milton Miller @ 2010-07-19 11:35 UTC (permalink / raw)
To: Alexander Graf; +Cc: Subrata Modak, linuxppc-dev, linux-kernel
In-Reply-To: <reloc-2010-07-19@mdm.bga.com>
I wrote:
> On Mon Jul 19 2010 at about 03:36:51 EST, Alexander Graf wrote:
> > On 19.07.2010, at 03:11, Benjamin Herrenschmidt wrote:
> >
> > > On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
> > > > commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
> > > > 2.6.34-rc6. However some other bad relocation warnings generated against
> > > > 2.6.35-rc5 on Power7/ppc64 below:
> > > >
> > > > MODPOST 2004 modules^M
> > > > WARNING: 2 bad relocations^M
> > > > c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
> > > > c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
> > >
> > > I think this is KVM + CONFIG_RELOCATABLE. Caused by:
> > >
> > > .global kvmppc_trampoline_lowmem
> > > kvmppc_trampoline_lowmem:
> > > .long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
> > >
> > > .global kvmppc_trampoline_enter
> > > kvmppc_trampoline_enter:
> > > .long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
> > >
> > > Alex, can you turn these into 64-bit on ppc64 so the relocator
> > > can grok them ?
> >
> > If I turn them into 64-bit, will the values be > RMA? In that case
> > things would break anyways. How does relocation work on PPC? Are the
> > first few megs copied over to low memory? Would I have to mask anything
> > in the above code to make sure I use the real values?
> >
> > Alex
> >
>
> You can still do the subtraction, but you have to allocate 64 bits for
> storage. Relocatable ppc64 kernels work by adjusting PPC64_RELOC_RELATIVE
> entries during early boot (reloc in reloc_64.S called from head_64.S).
>
> The code purposely only supports 64 bit relative addressing.
Oh yea, and for book-3s, the code copies from 0x100 to __end_interrupts
in arch/powerpc/kernel/exceptions-64s.h down to the real 0, but the rest
of the kernel is at some disjointed address. The interrupt will go to
the copy at the real zero. Any references to code outside that region
must be done via a full indrect branch (not a relative one), simiar to
the secondary startup (via following the function pointer in a descriptor
set in very low memory), or syscall entry and exception vectors via paca.
Book-3e (64 and 32 bit) are different. I forget how classic 32 bit works,
it may still have a < 32MB limitation.
milton
^ permalink raw reply
* Re: [PPC64/Power7 - 2.6.35-rc5] Bad relocation warnings whileBuilding a CONFIG_RELOCATABLE kernel with CONFIG_ISERIES enabled
From: Alexander Graf @ 2010-07-19 12:00 UTC (permalink / raw)
To: Milton Miller; +Cc: Subrata Modak, linuxppc-dev, linux-kernel
In-Reply-To: <reloc-2010-07-19-2@mdm.bga.com>
Milton Miller wrote:
> I wrote:
>
>> On Mon Jul 19 2010 at about 03:36:51 EST, Alexander Graf wrote:
>>
>>> On 19.07.2010, at 03:11, Benjamin Herrenschmidt wrote:
>>>
>>>
>>>> On Thu, 2010-07-15 at 17:05 +0530, Subrata Modak wrote:
>>>>
>>>>> commit e62cee42e66dcca83aae02748535f62e0f564a0c solved the problem for
>>>>> 2.6.34-rc6. However some other bad relocation warnings generated against
>>>>> 2.6.35-rc5 on Power7/ppc64 below:
>>>>>
>>>>> MODPOST 2004 modules^M
>>>>> WARNING: 2 bad relocations^M
>>>>> c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460^M
>>>>> c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598^M
>>>>>
>>>> I think this is KVM + CONFIG_RELOCATABLE. Caused by:
>>>>
>>>> .global kvmppc_trampoline_lowmem
>>>> kvmppc_trampoline_lowmem:
>>>> .long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
>>>>
>>>> .global kvmppc_trampoline_enter
>>>> kvmppc_trampoline_enter:
>>>> .long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
>>>>
>>>> Alex, can you turn these into 64-bit on ppc64 so the relocator
>>>> can grok them ?
>>>>
>>> If I turn them into 64-bit, will the values be > RMA? In that case
>>> things would break anyways. How does relocation work on PPC? Are the
>>> first few megs copied over to low memory? Would I have to mask anything
>>> in the above code to make sure I use the real values?
>>>
>>> Alex
>>>
>>>
>> You can still do the subtraction, but you have to allocate 64 bits for
>> storage. Relocatable ppc64 kernels work by adjusting PPC64_RELOC_RELATIVE
>> entries during early boot (reloc in reloc_64.S called from head_64.S).
>>
>> The code purposely only supports 64 bit relative addressing.
>>
>
> Oh yea, and for book-3s, the code copies from 0x100 to __end_interrupts
> in arch/powerpc/kernel/exceptions-64s.h down to the real 0, but the rest
> of the kernel is at some disjointed address. The interrupt will go to
> the copy at the real zero. Any references to code outside that region
> must be done via a full indrect branch (not a relative one), simiar to
> the secondary startup (via following the function pointer in a descriptor
> set in very low memory), or syscall entry and exception vectors via paca.
>
That would still break on normal PPC boxes, as any address accessed in
real mode has to be inside the RMA. And the #include for
kvm/book3s_rmhandlers.S happens after __end_interrupts. So I'd end up
with code that gets executed outside of the RMA after a relocation, right?
Alex
^ permalink raw reply
* [PATCH] KVM: PPC: Make long relocations be ulong
From: Alexander Graf @ 2010-07-19 14:54 UTC (permalink / raw)
To: kvm-ppc; +Cc: Linuxppc-dev, subrata, kvm
On Book3S KVM we directly expose some asm pointers to C code as
variables. These need to be relocated and thus break on relocatable
kernels.
To make sure we can at least build, let's mark them as long instead
of u32 where 64bit relocations don't work.
This fixes the following build error:
WARNING: 2 bad relocations
> c000000000008590 R_PPC64_ADDR32 .text+0x4000000000008460
> c000000000008594 R_PPC64_ADDR32 .text+0x4000000000008598
Please keep in mind that actually using KVM on a relocated kernel
might still break. This only fixes the compile problem.
Reported-by: Subrata Modak <subrata@linux.vnet.ibm.com>
Signed-off-by: Alexander Graf <agraf@suse.de>
---
I'm not sure which tree this should go through. Avi and Ben, please
coordinate this.
---
arch/powerpc/include/asm/kvm_book3s.h | 4 ++--
arch/powerpc/kvm/book3s_rmhandlers.S | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index b5b1961..b3d763d 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -131,8 +131,8 @@ extern void kvmppc_set_bat(struct kvm_vcpu *vcpu, struct kvmppc_bat *bat,
extern void kvmppc_giveup_ext(struct kvm_vcpu *vcpu, ulong msr);
extern int kvmppc_emulate_paired_single(struct kvm_run *run, struct kvm_vcpu *vcpu);
-extern u32 kvmppc_trampoline_lowmem;
-extern u32 kvmppc_trampoline_enter;
+extern ulong kvmppc_trampoline_lowmem;
+extern ulong kvmppc_trampoline_enter;
extern void kvmppc_rmcall(ulong srr0, ulong srr1);
extern void kvmppc_load_up_fpu(void);
extern void kvmppc_load_up_altivec(void);
diff --git a/arch/powerpc/kvm/book3s_rmhandlers.S b/arch/powerpc/kvm/book3s_rmhandlers.S
index 506d5c3..20eb908 100644
--- a/arch/powerpc/kvm/book3s_rmhandlers.S
+++ b/arch/powerpc/kvm/book3s_rmhandlers.S
@@ -242,10 +242,10 @@ define_load_up(vsx)
.global kvmppc_trampoline_lowmem
kvmppc_trampoline_lowmem:
- .long kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
+ PPC_LONG kvmppc_handler_lowmem_trampoline - CONFIG_KERNEL_START
.global kvmppc_trampoline_enter
kvmppc_trampoline_enter:
- .long kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
+ PPC_LONG kvmppc_handler_trampoline_enter - CONFIG_KERNEL_START
#include "book3s_segment.S"
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH] KVM: PPC: Make long relocations be ulong
From: Avi Kivity @ 2010-07-19 15:30 UTC (permalink / raw)
To: Alexander Graf; +Cc: Linuxppc-dev, subrata, kvm, kvm-ppc
In-Reply-To: <1279551293-32473-1-git-send-email-agraf@suse.de>
On 07/19/2010 05:54 PM, Alexander Graf wrote:
> I'm not sure which tree this should go through. Avi and Ben, please
> coordinate this.
> ---
> arch/powerpc/include/asm/kvm_book3s.h | 4 ++--
> arch/powerpc/kvm/book3s_rmhandlers.S | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
>
The diffstat suggests kvm.git.
Is this broken in Linus' tree, or just kvm.git?
--
error compiling committee.c: too many arguments to function
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Make long relocations be ulong
From: Alexander Graf @ 2010-07-19 15:33 UTC (permalink / raw)
To: Avi Kivity; +Cc: Linuxppc-dev, subrata, kvm, kvm-ppc
In-Reply-To: <4C446FA0.8050403@redhat.com>
Avi Kivity wrote:
> On 07/19/2010 05:54 PM, Alexander Graf wrote:
>> I'm not sure which tree this should go through. Avi and Ben, please
>> coordinate this.
>> ---
>> arch/powerpc/include/asm/kvm_book3s.h | 4 ++--
>> arch/powerpc/kvm/book3s_rmhandlers.S | 4 ++--
>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>
>>
>
> The diffstat suggests kvm.git.
>
> Is this broken in Linus' tree, or just kvm.git?
I got the bug report through linuxppc-dev, so I assume it's broken in
Linus' tree. In fact, that code has been around for a while already, so
it should even be in 2.6.24.
Alex
^ permalink raw reply
* Re: cpm_uart_console_write() stuck in waiting for transmitter fifo ready
From: Scott Wood @ 2010-07-19 15:36 UTC (permalink / raw)
To: Shawn Jin; +Cc: ppcdev
In-Reply-To: <AANLkTikyHvocHW6YzBPtxDrC9M5mih4ZpSoqo0mg6M1E@mail.gmail.com>
On Fri, 16 Jul 2010 17:30:01 -0700
Shawn Jin <shawnxjin@gmail.com> wrote:
> >> > My RCCR=3D0x1, meaning the first 512B is for microcode. So the data =
and
> >> > 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.
>=20
> It would be nicer that the initialization code could query the RCCR
> value and adjust the base address.
Is that the only thing that could possibly conflict with OS usage of
DPRAM? It seemed simpler to just specify the usable chunks.
Before the kernel supported allocating CPM2 SMC parameter RAM, that was
another thing that firmware allocated that had to be kept out of the
muram node.
> It took me quite a while to understand the design.
Sorry...
This constraint is described in the MURAM section of
Documentation/powerpc/dts-bindings/fsl/cpm_qe/cpm.txt, though.
-Scott
^ permalink raw reply
* Re: [PATCH V4] powerpc/prom: Export device tree physical address via proc
From: Scott Wood @ 2010-07-19 16:57 UTC (permalink / raw)
To: Grant Likely; +Cc: Matthew McClintock, Kumar Gala, linuxppc-dev, Timur Tabi
In-Reply-To: <AANLkTimJwnmf1YTPIy7OqdjseiUspzlxwRcjPMQ6Vv6j@mail.gmail.com>
On Sun, 18 Jul 2010 22:32:38 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:
> On Sun, Jul 18, 2010 at 10:28 PM, Matthew McClintock <msm@freescale.com> wrote:
> > Upon first examining the details of getting kexec working on our platform I noticed our device tree passed from u-boot contained an additional memreserve section for the boot page. Subsequently, I've been trying to preserve the ones passed in for the kexec'ed kernel thinking anyone could add anything they wanted for their own particular needs and would quite possibly need those regions preserved across reboots.
> >
> > Recently, I've discovered the boot page address is given in the device tree as a property. So, for our platform (mpc85xx) in particular, I'm back to not needing the read the old memreserve sections... I can completely reconstruct the appropriate memreserve regions for kexec'ing from the information passed in the device tree.
> >
> > That isn't to say there might not be more memreserve regions that are not there for some valid reason. I'm not sure if we need to attempt to address another scenario where there are other memreserve regions. So this would be a good time to address this issue if anyone believes it is a worthwhile pursuit to have a mechanism to have memreserve regions persistent across kexec'ing - or any other reboot.
>
> No, we haven't needed anything to date, so no sense trying to design a
> solution for a theoretical need. Leave it be for now.
So why do we have memreserve at all?
If we honor it on the first boot, seems like we should keep that
information around on subsequent boots. I wouldn't want to be the one
to have to debug when that theoretical need becomes actual. :-(
Or am I misunderstanding what this is trying to do?
-Scott
^ 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