Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
On Tue, Jan 12, 2010 at 12:06 AM, Roman Fietze 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 > -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
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 -- Roman FietzeTelemotive AG Büro Mühlhausen Breitwiesen 73347 Mühlhausen Tel.: +49(0)7335/18493-45http://www.telemotive.de Amtsgericht UlmHRB 541321 Vorstand: Peter Kersten, Markus Fischer, Franz Diller, Markus Stolz signature.asc Description: This is a digitally signed message part. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
On Mon, Jan 11, 2010 at 1:43 PM, Wolfgang Denk wrote: > Dear Grant, > > In message 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. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
Dear Grant, In message 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 -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Gods don't like people not doing much work. People who aren't busy all the time might start to _think_. - Terry Pratchett, _Small Gods_ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
On Mon, Jan 11, 2010 at 12:42 PM, Scott Wood 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. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
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 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
On Mon, Dec 21, 2009 at 11:59 PM, Roman Fietze
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
> ---
> 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.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 02/13] powerpc/5200: LocalPlus driver: use SCLPC register structure
Signed-off-by: Roman Fietze
---
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 */
+};
+
/* 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 ");
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, 0x0101);
+ out_be32(&lpbfifo.regs->enable, 0x0101);
/* set master enable bit */
- out_be32(lpbfifo.regs + LPBFIFO_REG_ENABLE, 0x0001);
+ out_be32(&lpbfifo.regs->enable, 0x0001);
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, 0x0301);
+ out_be32(&lpbfifo.regs->enable, 0x0301);
} 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(&lpb
