[PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable

[PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable

[Albrecht Dreß]

This patch adds a method for defining different microcodes than the pe-defi=
ned ones for the MPC52xx's BestComm General Buffer Descriptor (gen_db) task=
s.  The default microcode is still the one from bcom_gen_bd_[rt]x_task, but=
 it can be replaced by calling bcom_gen_bd_set_microcode() which is more ef=
ficient than explicitly loading it via bcom_load_image() after each bcom_ge=
n_bd_[rt]x_reset().

Except for a fixed tab vs. space formatting and the transfer format, there =
are no diffierences to the initial attempt.

Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>


---


diff -uprN -X linux-2.6.32-rc3/Documentation/dontdiff linux-2.6.32-rc3-orig=
/arch/powerpc/sysdev/bestcomm/gen_bd.c linux-2.6.32-rc3/arch/powerpc/sysdev=
/bestcomm/gen_bd.c
--- linux-2.6.32-rc3-orig/arch/powerpc/sysdev/bestcomm/gen_bd.c	2009-10-05 =
02:12:30.000000000 +0200
+++ linux-2.6.32-rc3/arch/powerpc/sysdev/bestcomm/gen_bd.c	2009-10-05 14:52=
:31.000000000 +0200
@@ -78,6 +78,7 @@ struct bcom_gen_bd_priv {
 	int		initiator;
 	int		ipr;
 	int		maxbufsize;
+	u32		*microcode;
 };
=20
=20
@@ -104,6 +105,7 @@ bcom_gen_bd_rx_init(int queue_len, phys_
 	priv->initiator	=3D initiator;
 	priv->ipr	=3D ipr;
 	priv->maxbufsize =3D maxbufsize;
+	priv->microcode =3D bcom_gen_bd_rx_task;
=20
 	if (bcom_gen_bd_rx_reset(tsk)) {
 		bcom_task_free(tsk);
@@ -128,7 +130,7 @@ bcom_gen_bd_rx_reset(struct bcom_task *t
 	var =3D (struct bcom_gen_bd_rx_var *) bcom_task_var(tsk->tasknum);
 	inc =3D (struct bcom_gen_bd_rx_inc *) bcom_task_inc(tsk->tasknum);
=20
-	if (bcom_load_image(tsk->tasknum, bcom_gen_bd_rx_task))
+	if (bcom_load_image(tsk->tasknum, priv->microcode))
 		return -1;
=20
 	var->enable	=3D bcom_eng->regs_base +
@@ -188,6 +190,7 @@ bcom_gen_bd_tx_init(int queue_len, phys_
 	priv->fifo	=3D fifo;
 	priv->initiator	=3D initiator;
 	priv->ipr	=3D ipr;
+	priv->microcode =3D bcom_gen_bd_tx_task;
=20
 	if (bcom_gen_bd_tx_reset(tsk)) {
 		bcom_task_free(tsk);
@@ -212,7 +215,7 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
 	var =3D (struct bcom_gen_bd_tx_var *) bcom_task_var(tsk->tasknum);
 	inc =3D (struct bcom_gen_bd_tx_inc *) bcom_task_inc(tsk->tasknum);
=20
-	if (bcom_load_image(tsk->tasknum, bcom_gen_bd_tx_task))
+	if (bcom_load_image(tsk->tasknum, priv->microcode))
 		return -1;
=20
 	var->enable	=3D bcom_eng->regs_base +
@@ -253,6 +256,15 @@ bcom_gen_bd_tx_release(struct bcom_task=20
 }
 EXPORT_SYMBOL_GPL(bcom_gen_bd_tx_release);
=20
+void
+bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode)
+{
+	struct bcom_gen_bd_priv *priv =3D tsk->priv;
+
+	priv->microcode =3D microcode;
+}
+EXPORT_SYMBOL_GPL(bcom_gen_bd_set_microcode);
+
 /* ---------------------------------------------------------------------
  * PSC support code
  */
diff -uprN -X linux-2.6.32-rc3/Documentation/dontdiff linux-2.6.32-rc3-orig=
/arch/powerpc/sysdev/bestcomm/gen_bd.h linux-2.6.32-rc3/arch/powerpc/sysdev=
/bestcomm/gen_bd.h
--- linux-2.6.32-rc3-orig/arch/powerpc/sysdev/bestcomm/gen_bd.h	2009-10-05 =
02:12:30.000000000 +0200
+++ linux-2.6.32-rc3/arch/powerpc/sysdev/bestcomm/gen_bd.h	2009-10-05 14:52=
:31.000000000 +0200
@@ -43,6 +43,9 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
 extern void
 bcom_gen_bd_tx_release(struct bcom_task *tsk);
=20
+extern void
+bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode);
+
=20
 /* PSC support utility wrappers */
 struct bcom_task * bcom_psc_gen_bd_rx_init(unsigned psc_num, int queue_len=
,

Re: [PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable

[Grant Likely]

On Mon, Oct 5, 2009 at 11:41 AM, Albrecht Dre=DF <albrecht.dress@arcor.de> =
wrote:
> This patch adds a method for defining different microcodes than the pe-de=
fined ones for the MPC52xx's BestComm General Buffer Descriptor (gen_db) ta=
sks. =A0The default microcode is still the one from bcom_gen_bd_[rt]x_task,=
 but it can be replaced by calling bcom_gen_bd_set_microcode() which is mor=
e efficient than explicitly loading it via bcom_load_image() after each bco=
m_gen_bd_[rt]x_reset().
>
> Except for a fixed tab vs. space formatting and the transfer format, ther=
e are no diffierences to the initial attempt.

Hmmm.  I've not been comfortable with this change, but it took me a
while to put my finger on exactly why.  In principle, I think it is a
good idea.  However, I don't want to merge it without any in-tree
users.

The other concern is that I don't like that the patch is only
applicable to gen_bd tasks.  I've been thinking about this for a
while, and I'm not exactly thrilled with the bestcomm layout which
keeps the bestcomm firmware separate from the only driver that
actually uses it (ie FEC firmware & support code is separate from the
FEC driver.  Same for ATA).  I don't like the fact that code which is
only ever used by the ATA driver is maintained completely separate
from it.  But I may be splitting hairs here and maybe I shouldn't be
too concerned about it.  Regardless, I'd think I'd rather see
something that isn't totally gen_bd specific.  Perhaps *microcode
should be part of the bcom_task structure?

Thoughts?

g.

>
> Signed-off-by: Albrecht Dre=DF <albrecht.dress@arcor.de>
>
>
> ---
>
>
> diff -uprN -X linux-2.6.32-rc3/Documentation/dontdiff linux-2.6.32-rc3-or=
ig/arch/powerpc/sysdev/bestcomm/gen_bd.c linux-2.6.32-rc3/arch/powerpc/sysd=
ev/bestcomm/gen_bd.c
> --- linux-2.6.32-rc3-orig/arch/powerpc/sysdev/bestcomm/gen_bd.c 2009-10-0=
5 02:12:30.000000000 +0200
> +++ linux-2.6.32-rc3/arch/powerpc/sysdev/bestcomm/gen_bd.c =A0 =A0 =A0200=
9-10-05 14:52:31.000000000 +0200
> @@ -78,6 +78,7 @@ struct bcom_gen_bd_priv {
> =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 initiator;
> =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 ipr;
> =A0 =A0 =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 maxbufsize;
> + =A0 =A0 =A0 u32 =A0 =A0 =A0 =A0 =A0 =A0 *microcode;
> =A0};
>
>
> @@ -104,6 +105,7 @@ bcom_gen_bd_rx_init(int queue_len, phys_
> =A0 =A0 =A0 =A0priv->initiator =3D initiator;
> =A0 =A0 =A0 =A0priv->ipr =A0 =A0 =A0 =3D ipr;
> =A0 =A0 =A0 =A0priv->maxbufsize =3D maxbufsize;
> + =A0 =A0 =A0 priv->microcode =3D bcom_gen_bd_rx_task;
>
> =A0 =A0 =A0 =A0if (bcom_gen_bd_rx_reset(tsk)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_task_free(tsk);
> @@ -128,7 +130,7 @@ bcom_gen_bd_rx_reset(struct bcom_task *t
> =A0 =A0 =A0 =A0var =3D (struct bcom_gen_bd_rx_var *) bcom_task_var(tsk->t=
asknum);
> =A0 =A0 =A0 =A0inc =3D (struct bcom_gen_bd_rx_inc *) bcom_task_inc(tsk->t=
asknum);
>
> - =A0 =A0 =A0 if (bcom_load_image(tsk->tasknum, bcom_gen_bd_rx_task))
> + =A0 =A0 =A0 if (bcom_load_image(tsk->tasknum, priv->microcode))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
>
> =A0 =A0 =A0 =A0var->enable =A0 =A0 =3D bcom_eng->regs_base +
> @@ -188,6 +190,7 @@ bcom_gen_bd_tx_init(int queue_len, phys_
> =A0 =A0 =A0 =A0priv->fifo =A0 =A0 =A0=3D fifo;
> =A0 =A0 =A0 =A0priv->initiator =3D initiator;
> =A0 =A0 =A0 =A0priv->ipr =A0 =A0 =A0 =3D ipr;
> + =A0 =A0 =A0 priv->microcode =3D bcom_gen_bd_tx_task;
>
> =A0 =A0 =A0 =A0if (bcom_gen_bd_tx_reset(tsk)) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0bcom_task_free(tsk);
> @@ -212,7 +215,7 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
> =A0 =A0 =A0 =A0var =3D (struct bcom_gen_bd_tx_var *) bcom_task_var(tsk->t=
asknum);
> =A0 =A0 =A0 =A0inc =3D (struct bcom_gen_bd_tx_inc *) bcom_task_inc(tsk->t=
asknum);
>
> - =A0 =A0 =A0 if (bcom_load_image(tsk->tasknum, bcom_gen_bd_tx_task))
> + =A0 =A0 =A0 if (bcom_load_image(tsk->tasknum, priv->microcode))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -1;
>
> =A0 =A0 =A0 =A0var->enable =A0 =A0 =3D bcom_eng->regs_base +
> @@ -253,6 +256,15 @@ bcom_gen_bd_tx_release(struct bcom_task
> =A0}
> =A0EXPORT_SYMBOL_GPL(bcom_gen_bd_tx_release);
>
> +void
> +bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode)
> +{
> + =A0 =A0 =A0 struct bcom_gen_bd_priv *priv =3D tsk->priv;
> +
> + =A0 =A0 =A0 priv->microcode =3D microcode;
> +}
> +EXPORT_SYMBOL_GPL(bcom_gen_bd_set_microcode);
> +
> =A0/* -------------------------------------------------------------------=
--
> =A0* PSC support code
> =A0*/
> diff -uprN -X linux-2.6.32-rc3/Documentation/dontdiff linux-2.6.32-rc3-or=
ig/arch/powerpc/sysdev/bestcomm/gen_bd.h linux-2.6.32-rc3/arch/powerpc/sysd=
ev/bestcomm/gen_bd.h
> --- linux-2.6.32-rc3-orig/arch/powerpc/sysdev/bestcomm/gen_bd.h 2009-10-0=
5 02:12:30.000000000 +0200
> +++ linux-2.6.32-rc3/arch/powerpc/sysdev/bestcomm/gen_bd.h =A0 =A0 =A0200=
9-10-05 14:52:31.000000000 +0200
> @@ -43,6 +43,9 @@ bcom_gen_bd_tx_reset(struct bcom_task *t
> =A0extern void
> =A0bcom_gen_bd_tx_release(struct bcom_task *tsk);
>
> +extern void
> +bcom_gen_bd_set_microcode(struct bcom_task *tsk, u32 *microcode);
> +
>
> =A0/* PSC support utility wrappers */
> =A0struct bcom_task * bcom_psc_gen_bd_rx_init(unsigned psc_num, int queue=
_len,
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable

[Albrecht Dreß]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

Am 05.10.09 20:16 schrieb(en) Grant Likely:
> Hmmm.  I've not been comfortable with this change, but it took me a while to put my finger on exactly why.  In principle, I think it is a good idea.  However, I don't want to merge it without any in-tree users.

I could provide my modified microcode (which differs in two words only), for the "standard" task, just with byte swapping for the LPB, but I wonder if it is of much use.  No progress with crc32 calculation in BestComm so far... :-(

> The other concern is that I don't like that the patch is only applicable to gen_bd tasks.

IMHO the big difference between gen_bd on one hand and ATA and FEC on the other is that (afaik) we have *no* other choice than using the implemented tasks for the latter two, obviously using the Freescale's example microcode.  Gen_bd can apparently be used for a plenty of purposes, including LPB (as shown in your driver), maybe PCI (there are examples in Freescale's code), PSC, etc.  Thus, the FEC and ATA drivers should not have an option as to change the microcode, whereas it is just convenient for gen_bd.

> I've been thinking about this for a while, and I'm not exactly thrilled with the bestcomm layout which keeps the bestcomm firmware separate from the only driver that actually uses it (ie FEC firmware & support code is separate from the FEC driver.  Same for ATA).  I don't like the fact that code which is only ever used by the ATA driver is maintained completely separate from it.

That's a good point!  The separate microcode files for ATA and FEC are somewhat confusing.  I guess it's a result of simply copying the Freescale examples into separate files (and that might be a good reason for keeping the microcode separated).  Maybe create new sub-folders below arch/powerpc/sysdev/bestcomm for fec, ata, and gen_bd?  At least some documentation would be great.

> But I may be splitting hairs here and maybe I shouldn't be too concerned about it.  Regardless, I'd think I'd rather see something that isn't totally gen_bd specific.  Perhaps *microcode should be part of the bcom_task structure?

See above - I believe FEC and ATA are somewhat special as they need *exactly* the provided microcode.  And if something cannot be changed without breaking the functionality, there should not be a chance to do so... ;-)

Just my €0.01, though...

Cheers, Albrecht.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH/v2] powerpc/5200: make BestComm gen_bd microcode exchangeable

[Grant Likely]

On Mon, Oct 5, 2009 at 1:42 PM, Albrecht Dre=DF <albrecht.dress@arcor.de> w=
rote:
> Am 05.10.09 20:16 schrieb(en) Grant Likely:
>>
>> Hmmm. =A0I've not been comfortable with this change, but it took me a wh=
ile
>> to put my finger on exactly why. =A0In principle, I think it is a good i=
dea.
>> =A0However, I don't want to merge it without any in-tree users.
>
> I could provide my modified microcode (which differs in two words only), =
for
> the "standard" task, just with byte swapping for the LPB, but I wonder if=
 it
> is of much use. =A0No progress with crc32 calculation in BestComm so far.=
..
> :-(
>
>> The other concern is that I don't like that the patch is only applicable
>> to gen_bd tasks.
>
> IMHO the big difference between gen_bd on one hand and ATA and FEC on the
> other is that (afaik) we have *no* other choice than using the implemente=
d
> tasks for the latter two, obviously using the Freescale's example microco=
de.
> =A0Gen_bd can apparently be used for a plenty of purposes, including LPB =
(as
> shown in your driver), maybe PCI (there are examples in Freescale's code)=
,
> PSC, etc. =A0Thus, the FEC and ATA drivers should not have an option as t=
o
> change the microcode, whereas it is just convenient for gen_bd.

agreed.

>> I've been thinking about this for a while, and I'm not exactly thrilled
>> with the bestcomm layout which keeps the bestcomm firmware separate from=
 the
>> only driver that actually uses it (ie FEC firmware & support code is
>> separate from the FEC driver. =A0Same for ATA). =A0I don't like the fact=
 that
>> code which is only ever used by the ATA driver is maintained completely
>> separate from it.
>
> That's a good point! =A0The separate microcode files for ATA and FEC are
> somewhat confusing. =A0I guess it's a result of simply copying the Freesc=
ale
> examples into separate files (and that might be a good reason for keeping
> the microcode separated). =A0Maybe create new sub-folders below
> arch/powerpc/sysdev/bestcomm for fec, ata, and gen_bd? =A0At least some
> documentation would be great.

No, that doesn't really help much.  The ATA and FEC microcode +
support functions really have no business living in the common
bestcomm directory at all.  I should look into moving that stuff right
into the FEC and ATA drivers.  The Bestcomm API is probably designed
to the wrong level, and the base API should expose the same task
loading interface to all uses, FEC, ATA, GenBD, and custom tasks.

>> But I may be splitting hairs here and maybe I shouldn't be too concerned
>> about it. =A0Regardless, I'd think I'd rather see something that isn't t=
otally
>> gen_bd specific. =A0Perhaps *microcode should be part of the bcom_task
>> structure?
>
> See above - I believe FEC and ATA are somewhat special as they need
> *exactly* the provided microcode. =A0And if something cannot be changed
> without breaking the functionality, there should not be a chance to do so=
...
> ;-)

Hence the FEC and ATA microcode should live in (*gasp*) the FEC and
ATA drivers.  ;-)

g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.