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

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(-)

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.

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

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.

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

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.

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

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
>