| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CACoXjc=tcZr_82JwfTzT+6re3d_e3pbn-yXvq=2ofbFSvSXM5g@mail.gmail.com>
Date: Tue, 29 Nov 2011 11:41:37 +0530
From: Tanmay Inamdar <tinamdar@....com>
To: Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc: Arnd Bergmann <arnd@...db.de>, linuxppc-dev@...ts.ozlabs.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] powerpc/40x: Add APM8018X SOC support
On Mon, Nov 28, 2011 at 4:49 AM, Benjamin Herrenschmidt
<benh@...nel.crashing.org> wrote:
> On Fri, 2011-11-25 at 17:49 +0530, Tanmay Inamdar wrote:
>
>>
>> >
>> > +#if defined(CONFIG_APM8018X)
>> > +/* CPR */
>> > +#define DCRN_CPR0_CONFIG_ADDR 0xa
>> > +#define DCRN_CPR1_CONFIG_DATA 0xb
>> > +/* AHB */
>> > +#define DCRN_SDR1_CONFIG_ADDR 0xc
>> > +#define DCRN_SDR1_CONFIG_DATA 0xd
>> > +#else
>> > /* CPRs (440GX and 440SP/440SPe) */
>> > #define DCRN_CPR0_CONFIG_ADDR 0xc
>> > #define DCRN_CPR0_CONFIG_DATA 0xd
>> > +#endif /* CONFIG_APM8018X */
>>
>>
>> same comment as above.
>>
>> Some existing drivers use these macros. If I change the names, I will
>> have to update the
>> driver code.
>
> Right, the best approach is to create a small wrapper that provides cpr
> and sdr accesses using helpers. That way you can:
>
> - Properly lock since it's all indirect
>
> - Obtain the right DCRs at boot time, stick them in variables
> and use them at runtime, avoiding the hard coded constants completely
>
> - Make the code generally look much better
>
> Ie. Provide something like read_sdr1() and write_sdr1() accessors and
> change the drivers to use them.
Ok.
There are 'mfdcri' and 'mtdcri' macros in
"arch/powerpc/include/asm/dcr-native.h" file.
They internally use '__mfdcri' and '__mtdcri' functions which can be
used for the same
purpose as mentioned above.
Instead of putting this change in current patch, I will make separate
patch for this purpose.
>
>> > diff --git a/arch/powerpc/kernel/cputable.c
>> b/arch/powerpc/kernel/cputable.c
>> > index edae5bb..e5c51a6 100644
>> > --- a/arch/powerpc/kernel/cputable.c
>> > +++ b/arch/powerpc/kernel/cputable.c
>> > @@ -1505,6 +1505,58 @@ static struct cpu_spec __initdata
>> cpu_specs[] = {
>> > .machine_check = machine_check_4xx,
>> > .platform = "ppc405",
>> > },
>> > + { /* APM80186-SK */
>> > + .pvr_mask = 0xffffffff,
>> > + .pvr_value = 0x7ff11432,
>>
>>
>> If you mask out the lower bits, you only need a single entry
>> instead of
>> four identical ones.
>>
>> You are right. But each PVR represent different version of SOC. If I
>> create single entry, then I will have to give generic cpu_name which I
>> don't want.
>
> Note that you should really tell you designers to move away from using
> the PVR to identify the SoC's. This is BAD and isn't sustainable long
> run. Stick to representing the core itself in the PVR and provide a
> different mechanism to identify the SoC (different SPR would do, or even
> a DCR).
>
>> > --- a/arch/powerpc/kernel/udbg_16550.c
>> > +++ b/arch/powerpc/kernel/udbg_16550.c
>> > @@ -18,6 +18,19 @@ extern void real_writeb(u8 data, volatile
>> u8 __iomem *addr);
>> > extern u8 real_205_readb(volatile u8 __iomem *addr);
>> > extern void real_205_writeb(u8 data, volatile u8 __iomem
>> *addr);
>> >
>> > +#ifdef CONFIG_UART_16550_WORD_ADDRESSABLE
>> > +struct NS16550 {
>> > + /* this struct must be packed */
>> > + unsigned char rbr; /* 0 */ u8 s0[3];
>> > + unsigned char ier; /* 1 */ u8 s1[3];
>> > + unsigned char fcr; /* 2 */ u8 s2[3];
>> > + unsigned char lcr; /* 3 */ u8 s3[3];
>> > + unsigned char mcr; /* 4 */ u8 s4[3];
>> > + unsigned char lsr; /* 5 */ u8 s5[3];
>> > + unsigned char msr; /* 6 */ u8 s6[3];
>> > + unsigned char scr; /* 7 */ u8 s7[3];
>> > +};
>> > +#else
>> > struct NS16550 {
>> > /* this struct must be packed */
>> > unsigned char rbr; /* 0 */
>> > @@ -29,6 +42,7 @@ struct NS16550 {
>> > unsigned char msr; /* 6 */
>> > unsigned char scr; /* 7 */
>> > };
>> > +#endif /* CONFIG_UART_16550_WORD_ADDRESSABLE */
>> >
>>
>>
>> Same things as with the register definitions. Please make this
>> run-time selectable to allow build-time configurations that
>> support both layouts.
>>
>> Yes. I will try to find better solution.
>
> Cheers,
> Ben.
>
>
>
Regards,
Tanmay
CONFIDENTIALITY NOTICE: This e-mail message, including any attachments,
is for the sole use of the intended recipient(s) and contains information
that is confidential and proprietary to AppliedMicro Corporation or its subsidiaries.
It is to be used solely for the purpose of furthering the parties' business relationship.
All unauthorized review, use, disclosure or distribution is prohibited.
If you are not the intended recipient, please contact the sender by reply e-mail
and destroy all copies of the original message.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists