Patchwork [02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure

login
register
mail settings
Submitter Roman Fietze
Date Dec. 22, 2009, 6:59 a.m.
Message ID <200912220759.23453.roman.fietze@telemotive.de>
Download mbox | patch
Permalink /patch/41585/
State Changes Requested
Delegated to: Grant Likely
Headers show

Comments

Roman Fietze - Dec. 22, 2009, 6:59 a.m.
Signed-off-by: Roman Fietze <roman.fietze@telemotive.de>
---
 arch/powerpc/include/asm/mpc52xx.h            |   24 ++++++++
 arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   79 +++++++++++--------------
 2 files changed, 59 insertions(+), 44 deletions(-)
Grant Likely - Jan. 11, 2010, 7:15 p.m.
On Mon, Dec 21, 2009 at 11:59 PM, Roman Fietze
<roman.fietze@telemotive.de> wrote:

No patch description?  Very few patches are sufficiently described by
the subject line alone.  Tell me what the problem is, what the patch
changes, and why.

> Signed-off-by: Roman Fietze <roman.fietze@telemotive.de>
> ---
>  arch/powerpc/include/asm/mpc52xx.h            |   24 ++++++++
>  arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c |   79 +++++++++++--------------
>  2 files changed, 59 insertions(+), 44 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mpc52xx.h b/arch/powerpc/include/asm/mpc52xx.h
> index b664ce7..57f8335 100644
> --- a/arch/powerpc/include/asm/mpc52xx.h
> +++ b/arch/powerpc/include/asm/mpc52xx.h
> @@ -193,6 +193,30 @@ struct mpc52xx_xlb {
>  #define MPC52xx_XLB_CFG_PLDIS          (1 << 31)
>  #define MPC52xx_XLB_CFG_SNOOP          (1 << 15)
>
> +/* SCLPC */
> +struct mpc52xx_sclpc {
> +       union {
> +               u8 restart;     /* 0x00 restart bit */
> +               u32 packet_size; /* 0x00 packet size register */
> +       } packet_size;
> +       u32 start_address;      /* 0x04 start Address register */
> +       u32 control;            /* 0x08 control register */
> +       u32 enable;             /* 0x0C enable register */
> +       u32 unused0;            /* 0x10 */
> +       union {
> +               u8 status;      /* 0x14 status register bits */
> +               u32 bytes_done; /* 0x14 bytes done register bits, read only */
> +       } bytes_done_status;
> +
> +       u32 reserved1[(0x40-0x18) / sizeof(u32)];       /* 0x18 .. 0x3c */
> +
> +       u32 fifo_data;          /* 0x40 FIFO data word register */
> +       u32 fifo_status;        /* 0x44 FIFO status register */
> +       u8 fifo_control;        /* 0x48 FIFO control register */
> +       u8 reserved2[3];
> +       u32 fifo_alarm;         /* 0x4C FIFO alarm register */
> +};

Please don't.  I know that a lot of other 5200 code uses register map
structures in this way, but I consider it bad practice.  I coded this
driver without a structure for a reason.  The reason I haven't removed
the other 5200 register map structures is the code impact would be
huge, it would probably cause breakage, and it would break all
out-of-tree patches touching the same code for no measurable
advantage.

For this code, there is no advantage to changing the register access
method, and it generates more work in other areas.  Even if I
preferred register map structures I would resist applying this patch.

Please drop from your series.

g.
Scott Wood - Jan. 11, 2010, 7:42 p.m.
Grant Likely wrote:
> Please don't.  I know that a lot of other 5200 code uses register map
> structures in this way, but I consider it bad practice.  I coded this
> driver without a structure for a reason.  The reason I haven't removed
> the other 5200 register map structures is the code impact would be
> huge, it would probably cause breakage, and it would break all
> out-of-tree patches touching the same code for no measurable
> advantage.

FWIW, over on the U-Boot side patches are getting NACKed by Wolfgang if 
they don't use register structures. :-P

They're nice from a type-safety and namespacing perspective, though they 
get ugly pretty quickly if there are gaps.

-Scott
Grant Likely - Jan. 11, 2010, 7:59 p.m.
On Mon, Jan 11, 2010 at 12:42 PM, Scott Wood <scottwood@freescale.com> wrote:
> Grant Likely wrote:
>>
>> Please don't.  I know that a lot of other 5200 code uses register map
>> structures in this way, but I consider it bad practice.  I coded this
>> driver without a structure for a reason.  The reason I haven't removed
>> the other 5200 register map structures is the code impact would be
>> huge, it would probably cause breakage, and it would break all
>> out-of-tree patches touching the same code for no measurable
>> advantage.
>
> FWIW, over on the U-Boot side patches are getting NACKed by Wolfgang if they
> don't use register structures. :-P
>
> They're nice from a type-safety and namespacing perspective, though they get
> ugly pretty quickly if there are gaps.

Regardless, I see no reason to change existing code in either direction.

g.
Wolfgang Denk - Jan. 11, 2010, 8:43 p.m.
Dear Grant,

In message <fa686aa41001111115g3451c9b9h4d6c551afd5698e1@mail.gmail.com> you wrote:
> 
> Please don't.  I know that a lot of other 5200 code uses register map
> structures in this way, but I consider it bad practice.  I coded this

May I ask _why_ you consider this bad practice?

Is a structure not the most natural way to encode the specifics of a
hardware interface (address offet, bus width, etc.) in C?

What do you recommend instead?  Using lists of register offsets
(without any type information) as for example ARM is doing?

> driver without a structure for a reason.  The reason I haven't removed

Could you please explain this reason?


I'm trying to understand if this is a MPC52xx specific reasoning, or
if you apply this to all of PowerPC, or generally to all kernel code?

And: is this just your personal preferences, or generally agreed on?

Thanks in advance, and sorry for asking stupid questions, but your
reply surprised me...

Best regards,

Wolfgang Denk
Grant Likely - Jan. 11, 2010, 9:20 p.m.
On Mon, Jan 11, 2010 at 1:43 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> In message <fa686aa41001111115g3451c9b9h4d6c551afd5698e1@mail.gmail.com> you wrote:
>>
>> Please don't.  I know that a lot of other 5200 code uses register map
>> structures in this way, but I consider it bad practice.  I coded this
>
> May I ask _why_ you consider this bad practice?

Many reasons.  First off, while C structures somewhat represent the
layout of a hardware register set, I still find them a poor fit.
Registers do not data structures, and trying to describe them as such
causes problems.  Not all devices get mapped onto the bus in the same
way.  ie. a single device can get wired up with 8-bit wide addressing
on one system and 32 wide on another.  A struct cannot encode this.  I
also find I often need to access registers at "none-native" widths due
to implementation details of the device which is made messy when the
layout is encoded in a C struct.  Finally, we're talking about a
hardware interface here.  Driver authors must understand exactly what
they are doing when writing to registers and it is my opinion (though
others may disagree with me) that using structs to describe register
maps encourages a glosses over details that are best left explicit.

I used to prefer C structs for register definitions, but my opinion
changed as I gained more experience.

> Is a structure not the most natural way to encode the specifics of a
> hardware interface (address offet, bus width, etc.) in C?
>
> What do you recommend instead?  Using lists of register offsets
> (without any type information) as for example ARM is doing?

Yes, that is what I prefer.  That and, when needed, device-specific
accessor functions that can be adapted for different bus attachements.

>> driver without a structure for a reason.  The reason I haven't removed
>
> Could you please explain this reason?

As described above.

> I'm trying to understand if this is a MPC52xx specific reasoning, or
> if you apply this to all of PowerPC, or generally to all kernel code?

I've stated what I prefer.  I don't think I've rejected any code that
uses structs over register offsets, but I do apply friction on any
patch that changes a current driver from one to the other without
need.

> And: is this just your personal preferences, or generally agreed on?

Others will need to answer that.
Roman Fietze - Jan. 12, 2010, 7:06 a.m.
Hello Grant,

On Monday 11 January 2010 20:15:58 Grant Likely wrote:

> No patch description?  Very few patches are sufficiently described by
> the subject line alone.  Tell me what the problem is, what the patch
> changes, and why.

And a lot of other information. Thank you very much for the effort you
put into those patches. I almost knew that you will criticize e.g.
patches that mix functional and "optical" changes, patches that are
not describing more exactly what I did.

As soon as I have some time left I will try to go once more from the
original driver to the final driver using clear and separate, well
bescribed patches, of course taking care of your and other people
comments.

Again, thanks


Roman
Grant Likely - Jan. 12, 2010, 2:33 p.m.
On Tue, Jan 12, 2010 at 12:06 AM, Roman Fietze
<roman.fietze@telemotive.de> wrote:
> Hello Grant,
>
> On Monday 11 January 2010 20:15:58 Grant Likely wrote:
>
>> No patch description?  Very few patches are sufficiently described by
>> the subject line alone.  Tell me what the problem is, what the patch
>> changes, and why.
>
> And a lot of other information. Thank you very much for the effort you
> put into those patches. I almost knew that you will criticize e.g.
> patches that mix functional and "optical" changes, patches that are
> not describing more exactly what I did.
>
> As soon as I have some time left I will try to go once more from the
> original driver to the final driver using clear and separate, well
> bescribed patches, of course taking care of your and other people
> comments.

Sounds good.  I look forward to seeing them.

g.

>
> Again, thanks
>
>
> Roman
>
> --
> Roman Fietze                Telemotive AG Büro Mühlhausen
> Breitwiesen                              73347 Mühlhausen
> Tel.: +49(0)7335/18493-45        http://www.telemotive.de
>
> Amtsgericht Ulm                                HRB 541321
> Vorstand:
> Peter Kersten, Markus Fischer, Franz Diller, Markus Stolz
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
>

Patch

diff --git a/arch/powerpc/include/asm/mpc52xx.h b/arch/powerpc/include/asm/mpc52xx.h
index b664ce7..57f8335 100644
--- a/arch/powerpc/include/asm/mpc52xx.h
+++ b/arch/powerpc/include/asm/mpc52xx.h
@@ -193,6 +193,30 @@  struct mpc52xx_xlb {
 #define MPC52xx_XLB_CFG_PLDIS		(1 << 31)
 #define MPC52xx_XLB_CFG_SNOOP		(1 << 15)
 
+/* SCLPC */
+struct mpc52xx_sclpc {
+	union {
+		u8 restart;	/* 0x00 restart bit */
+		u32 packet_size; /* 0x00 packet size register */
+	} packet_size;
+	u32 start_address;	/* 0x04	start Address register */
+	u32 control;		/* 0x08	control register */
+	u32 enable;		/* 0x0C	enable register */
+	u32 unused0;		/* 0x10 */
+	union {
+		u8 status;	/* 0x14	status register bits */
+		u32 bytes_done; /* 0x14	bytes done register bits, read only */
+	} bytes_done_status;
+
+	u32 reserved1[(0x40-0x18) / sizeof(u32)];	/* 0x18 .. 0x3c */
+
+	u32 fifo_data;		/* 0x40	FIFO data word register */
+	u32 fifo_status;	/* 0x44	FIFO status register */
+	u8 fifo_control;	/* 0x48	FIFO control register */
+	u8 reserved2[3];
+	u32 fifo_alarm;		/* 0x4C	FIFO alarm register */
+};
+
 /* Clock Distribution control */
 struct mpc52xx_cdm {
 	u32 jtag_id;		/* CDM + 0x00  reg0 read only */
diff --git a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
index 4c84aa5..2763d5e 100644
--- a/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
+++ b/arch/powerpc/platforms/52xx/mpc52xx_lpbfifo.c
@@ -27,20 +27,10 @@  MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
 MODULE_DESCRIPTION("MPC5200 LocalPlus FIFO device driver");
 MODULE_LICENSE("GPL");
 
-#define LPBFIFO_REG_PACKET_SIZE		(0x00)
-#define LPBFIFO_REG_START_ADDRESS	(0x04)
-#define LPBFIFO_REG_CONTROL		(0x08)
-#define LPBFIFO_REG_ENABLE		(0x0C)
-#define LPBFIFO_REG_BYTES_DONE_STATUS	(0x14)
-#define LPBFIFO_REG_FIFO_DATA		(0x40)
-#define LPBFIFO_REG_FIFO_STATUS		(0x44)
-#define LPBFIFO_REG_FIFO_CONTROL	(0x48)
-#define LPBFIFO_REG_FIFO_ALARM		(0x4C)
-
 struct mpc52xx_lpbfifo {
 	struct device *dev;
 	phys_addr_t regs_phys;
-	void __iomem *regs;
+	struct mpc52xx_sclpc __iomem *regs;
 	int irq;
 	spinlock_t lock;
 
@@ -72,10 +62,10 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 	int poll_dma = req->flags & MPC52XX_LPBFIFO_FLAG_POLL_DMA;
 
 	/* Set and clear the reset bits; is good practice in User Manual */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000);
+	out_be32(&lpbfifo.regs->enable, 0x01010000);
 
 	/* set master enable bit */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x00000001);
+	out_be32(&lpbfifo.regs->enable, 0x00000001);
 	if (!dma) {
 		/* While the FIFO can be setup for transfer sizes as large as
 		 * 16M-1, the FIFO itself is only 512 bytes deep and it does
@@ -91,14 +81,14 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 
 		/* Load the FIFO with data */
 		if (write) {
-			reg = lpbfifo.regs + LPBFIFO_REG_FIFO_DATA;
+			reg = &lpbfifo.regs->fifo_data;
 			data = req->data + req->pos;
 			for (i = 0; i < transfer_size; i += 4)
 				out_be32(reg, *data++);
 		}
 
 		/* Unmask both error and completion irqs */
-		out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x00000301);
+		out_be32(&lpbfifo.regs->enable, 0x00000301);
 	} else {
 		/* Choose the correct direction
 		 *
@@ -108,12 +98,12 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 		 * is a risk of DMA not transferring the last chunk of data
 		 */
 		if (write) {
-			out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x1e4);
-			out_8(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 7);
+			out_be32(&lpbfifo.regs->fifo_alarm, 0x1e4);
+			out_8(&lpbfifo.regs->fifo_control, 7);
 			lpbfifo.bcom_cur_task = lpbfifo.bcom_tx_task;
 		} else {
-			out_be32(lpbfifo.regs + LPBFIFO_REG_FIFO_ALARM, 0x1ff);
-			out_8(lpbfifo.regs + LPBFIFO_REG_FIFO_CONTROL, 0);
+			out_be32(&lpbfifo.regs->fifo_alarm, 0x1ff);
+			out_8(&lpbfifo.regs->fifo_control, 0);
 			lpbfifo.bcom_cur_task = lpbfifo.bcom_rx_task;
 
 			if (poll_dma) {
@@ -155,21 +145,21 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 		/* Unmask irqs */
 		if (write && (!poll_dma))
 			bit_fields |= 0x00000100; /* completion irq too */
-		out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, bit_fields);
+		out_be32(&lpbfifo.regs->enable, bit_fields);
 	}
 
 	/* Set transfer size, width, chip select and READ mode */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_START_ADDRESS,
+	out_be32(&lpbfifo.regs->start_address,
 		 req->offset + req->pos);
-	out_be32(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, transfer_size);
+	out_be32(&lpbfifo.regs->packet_size.packet_size, transfer_size);
 
 	bit_fields = req->cs << 24 | 0x000008;
 	if (!write)
 		bit_fields |= 0x010000;	/* read mode */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_CONTROL, bit_fields);
+	out_be32(&lpbfifo.regs->control, bit_fields);
 
 	/* Kick it off */
-	out_8(lpbfifo.regs + LPBFIFO_REG_PACKET_SIZE, 0x01);
+	out_8(&lpbfifo.regs->packet_size.restart, 0x01);
 	if (dma)
 		bcom_enable(lpbfifo.bcom_cur_task);
 }
@@ -218,7 +208,7 @@  static void mpc52xx_lpbfifo_kick(struct mpc52xx_lpbfifo_request *req)
 static irqreturn_t mpc52xx_lpbfifo_irq(int irq, void *dev_id)
 {
 	struct mpc52xx_lpbfifo_request *req;
-	u32 status = in_8(lpbfifo.regs + LPBFIFO_REG_BYTES_DONE_STATUS);
+	u32 status = in_8(&lpbfifo.regs->bytes_done_status.status);
 	void __iomem *reg;
 	u32 *data;
 	int count, i;
@@ -253,18 +243,18 @@  static irqreturn_t mpc52xx_lpbfifo_irq(int irq, void *dev_id)
 
 	/* check abort bit */
 	if (status & 0x10) {
-		out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000);
+		out_be32(&lpbfifo.regs->enable, 0x01010000);
 		do_callback = 1;
 		goto out;
 	}
 
 	/* Read result from hardware */
-	count = in_be32(lpbfifo.regs + LPBFIFO_REG_BYTES_DONE_STATUS);
+	count = in_be32(&lpbfifo.regs->bytes_done_status.bytes_done);
 	count &= 0x00ffffff;
 
 	if (!dma && !write) {
 		/* copy the data out of the FIFO */
-		reg = lpbfifo.regs + LPBFIFO_REG_FIFO_DATA;
+		reg = &lpbfifo.regs->fifo_data;
 		data = req->data + req->pos;
 		for (i = 0; i < count; i += 4)
 			*data++ = in_be32(reg);
@@ -281,7 +271,7 @@  static irqreturn_t mpc52xx_lpbfifo_irq(int irq, void *dev_id)
 
 out:
 	/* Clear the IRQ */
-	out_8(lpbfifo.regs + LPBFIFO_REG_BYTES_DONE_STATUS, 0x01);
+	out_8(&lpbfifo.regs->bytes_done_status.status, 0x01);
 
 	if (dma && (status & 0x11)) {
 		/*
@@ -379,11 +369,11 @@  void mpc52xx_lpbfifo_poll(void)
 	int write = req->flags & MPC52XX_LPBFIFO_FLAG_WRITE;
 
 	/*
-	 * For more information, see comments on the "Fat Lady"
+	 * For more information, see comments on the "Fat Lady" 
 	 */
 	if (dma && write)
 		mpc52xx_lpbfifo_irq(0, NULL);
-	else
+	else 
 		mpc52xx_lpbfifo_bcom_irq(0, NULL);
 }
 EXPORT_SYMBOL(mpc52xx_lpbfifo_poll);
@@ -429,7 +419,7 @@  void mpc52xx_lpbfifo_abort(struct mpc52xx_lpbfifo_request *req)
 		/* Put it into reset and clear the state */
 		bcom_gen_bd_rx_reset(lpbfifo.bcom_rx_task);
 		bcom_gen_bd_tx_reset(lpbfifo.bcom_tx_task);
-		out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000);
+		out_be32(&lpbfifo.regs->enable, 0x01010000);
 		lpbfifo.req = NULL;
 	}
 	spin_unlock_irqrestore(&lpbfifo.lock, flags);
@@ -459,19 +449,19 @@  mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
 	spin_lock_init(&lpbfifo.lock);
 
 	/* Put FIFO into reset */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000);
+	out_be32(&lpbfifo.regs->enable, 0x01010000);
 
-	/* Register the interrupt handler */
+	/* register the interrupt handler */
 	rc = request_irq(lpbfifo.irq, mpc52xx_lpbfifo_irq, 0,
 			 "mpc52xx-lpbfifo", &lpbfifo);
 	if (rc)
 		goto err_irq;
 
 	/* Request the Bestcomm receive (fifo --> memory) task and IRQ */
-	lpbfifo.bcom_rx_task =
-		bcom_gen_bd_rx_init(2, res.start + LPBFIFO_REG_FIFO_DATA,
-				    BCOM_INITIATOR_SCLPC, BCOM_IPR_SCLPC,
-				    16*1024*1024);
+	lpbfifo.bcom_rx_task = bcom_gen_bd_rx_init(2,
+						   res.start + offsetof(struct mpc52xx_sclpc, fifo_data),
+						   BCOM_INITIATOR_SCLPC, BCOM_IPR_SCLPC,
+						   16*1024*1024);
 	if (!lpbfifo.bcom_rx_task)
 		goto err_bcom_rx;
 
@@ -482,9 +472,10 @@  mpc52xx_lpbfifo_probe(struct of_device *op, const struct of_device_id *match)
 		goto err_bcom_rx_irq;
 
 	/* Request the Bestcomm transmit (memory --> fifo) task and IRQ */
-	lpbfifo.bcom_tx_task =
-		bcom_gen_bd_tx_init(2, res.start + LPBFIFO_REG_FIFO_DATA,
-				    BCOM_INITIATOR_SCLPC, BCOM_IPR_SCLPC);
+	lpbfifo.bcom_tx_task = bcom_gen_bd_tx_init(2,
+						   res.start + offsetof(struct mpc52xx_sclpc, fifo_data),
+						   BCOM_INITIATOR_SCLPC,
+						   BCOM_IPR_SCLPC);
 	if (!lpbfifo.bcom_tx_task)
 		goto err_bcom_tx;
 
@@ -511,12 +502,12 @@  static int __devexit mpc52xx_lpbfifo_remove(struct of_device *op)
 		return 0;
 
 	/* Put FIFO in reset */
-	out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x01010000);
+	out_be32(&lpbfifo.regs->enable, 0x01010000);
 
-	/* Release the bestcomm transmit task */
+	/* release the bestcomm transmit task */
 	free_irq(bcom_get_task_irq(lpbfifo.bcom_tx_task), &lpbfifo);
 	bcom_gen_bd_tx_release(lpbfifo.bcom_tx_task);
-
+	
 	/* Release the bestcomm receive task */
 	free_irq(bcom_get_task_irq(lpbfifo.bcom_rx_task), &lpbfifo);
 	bcom_gen_bd_rx_release(lpbfifo.bcom_rx_task);