Re: [linuxsh-dev] Patch: Boot time configuration of SCI(F) and Overdrive support

[Mitch D. <mj...@au...>]

Hi Stuart!

Stuart Menefy wrote:
> 
> I've uploaded a small patch to patch manager on SourceForge (and a
> copy is also included in this email)

Cool, thanks.  BTW I like the feature list of this patch.

> Unfortunatly some of this clashes with the patch generated by
> Pierre-Philippe. I think we had similar objectives for the SCI
> changes, although his patch also contains quite a few other things
> which are specific to his hardware. IMHO this way of doing things
> is a bit more general.

General is good.  There are two ways to work this out.
The first is to choose one patch, check it in first, then the
other person has the job of resolving conflicts and making
sure their code works too.  They will then resubmit this patch.
You'll need to work out who goes first with Pierre.

The second is to get in touch with Pierre and privately
work out how you both want the code to look and work.
When you both have something unified that you like, that's
what gets submitted.

Another idea is to submit smaller non-clashing patches
which do less.  This will tend to just leave the areas which
really do clash and you and he can concentrate on these areas.
An example might be splitting this patch into two - a patch
for the serial stuff, and a patch to add support for the
STM OverDrive board.

Note, normally this problem would not have arisen, because
patches like Pierre's would have been applied much earlier,
instead of being left unhandled.

At the time Pierre sent this patch, I felt it had changes for
his hardware which were not sufficiently wrapped to isolate
them from the generic code, so I didn't push for its inclusion.
But it _is_ my fault for not communicating that to him so he
could revise it.  Sorry Pierre!

Soon I will have some serious time to spend on this project,
and this sort of problem will be less likely to reoccur.

> By the way, Pierre's patch on sourceforge appears to be corrupt.

I put that patch there by saving the mail that was sent to the
mailing list.  It's possible Netscape mangled it at Save As
time.  I've noted both the fixing of the corruption, and
the resolving of his patch in my TO-DO list.  

Alternatively, Pierre, could you put a new version of your patch
on SourceForge please?  Here's the URL:

  http://sourceforge.net/patch/?func=detailpatch&patch_id=100369&group_id=2682

Stuart, at m17n we discussed public discussion of patches, so
hope you don't mind my comments.

> +if [ "$CONFIG_SH_OVERDRIVE" = "n" ]; then
> +  choice 'Processor type' \
> +     "SH7708 CONFIG_CPU_SUBTYPE_SH7708 \
> +      SH7709 CONFIG_CPU_SUBTYPE_SH7709 \
> +      SH7750 CONFIG_CPU_SUBTYPE_SH7750" SH7708
> +  if [ "$CONFIG_CPU_SUBTYPE_SH7708" = "y" ]; then
> +     define_bool CONFIG_CPU_SH3 y
> +     define_bool CONFIG_CPU_SH4 n
> +  fi

One thing which occurs to me is that the Alpha people have
this problem, but even more so.  That is, dozens and dozens
of different boards, each with different CPUs and slightly
different peripherals.  It might be worth having a look at
how they've done things to see if there are ideas worth 
stealing.

> +++ kernel/arch/sh/kernel/setup.c       2000/05/24 10:44:03
> +#define PRINT_CLOCK(name, value) \
> +       p += sprintf(p, name " clock: %d.%02dMHz\n", \
> +                    ((value) / 1000000), ((value) % 1000000)/10000)
> +
> +       PRINT_CLOCK("CPU", boot_cpu_data.cpu_clock);
> +       PRINT_CLOCK("Bus", boot_cpu_data.bus_clock);
> +       PRINT_CLOCK("Peripherial module", boot_cpu_data.module_clock);

s/Peripherial/Peripheral/

> --- kernel/drivers/char/sh-sci.c        2000/05/22 09:41:31     1.8
>         } else {
> -               sci_setsignals (port, 0, -1);
> +               /* sci_setsignals (port, 0, -1); */
>         }

Just out of curiosity, what did sci_setsignals do?

> -#if defined(__sh3__)
> -#if defined(CONFIG_CPU_SUBTYPE_SH7709)
> -#define PCLK           33333333
> -#else
> -#define PCLK           14745600        /* Isn't it 15MHz? */
> -#endif
> -#elif defined(__SH4__)
> -#define PCLK           33333333
> -#endif
> +#define PCLK           (current_cpu_data.module_clock)

This is waaaaay cool! :-)

> +++ kernel/arch/sh/kernel/setup_od.c   Tue May 23 22:15:48 2000
> @@ -0,0 +1,35 @@
> +/* $Id$
> + *
> + * linux/arch/sh/kernel/setup_od.c
> + *
> + * Copyright (C) 2000  Stuart Menefy
> + *
> + * STMicroelectronics Overdrive Support.

Just a general note to people doing work while in the employ
of companies: To protect yourself, your employer and the Linux
kernel, it might be a good idea to get something signed to say
that your company doesn't regard the code as its intellectual
property.  Saying it's licensed under the GPL can't hurt either.

> +int __init setup_od(void)
> +{
> +       /* Enable RS232 receive buffers */
> +       volatile int* p = (volatile int*)0xa3000000;
> +
> +#if defined(CONFIG_SH_ORION)

Is this defined elsewhere?  Would you like it to be?

-- 

In all, a nice patch.  Thank you.

Regards,

Mitch.
--
| mailto:mj...@au...       | Not the official view of: |
| mailto:mj...@al... | Australian Calculator Opn |
| Certified Linux Evangelist! | Hewlett Packard Australia |