Proper way to implement platform specific quirk

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Proper way to implement platform specific quirk

Edwin Amsler
Hey there,

This e-mail is about design and custom arch-specific code.

I’m trying to author a patch from Bitrig to enable proper support for AHCI in the SUNXI ARMv7 port. There’s a bit of a quirk in how SUNXI handles DMA from what I understand of the patch. I think it may only affect the A10, but that’s outside the scope of this e-mail.

The fix is a series of patches, but the first one is going to be the tough since the change nestled inside ahci_port_start() in sys/dev/ic/ahci.c. Here’s the web-view of the patch: (let me know if you need me to provide a diff for readability)
https://github.com/bitrig/bitrig/commit/6342a27bfe4dc590f8e266e370f54846a766a737

Now, I don’t care about the original author’s implementation choice, everyone is justified in their own actions. What I want to know is what needs to be done to increase the chances this will be committed by you folks.

To be honest, I’d prefer to #ifdef the custom initialization based on target platform and clean up the ahci_softc.sc_flags bit field. Is everyone on board for that? Is there a better way?

Thanks,

Edwin Amsler

Reply | Threaded
Open this post in threaded view
|

Re: Proper way to implement platform specific quirk

Jonathan Gray-11
On Sat, Nov 22, 2014 at 02:06:48PM -0600, Edwin Amsler wrote:

> Hey there,
>
> This e-mail is about design and custom arch-specific code.
>
> I?m trying to author a patch from Bitrig to enable proper support for AHCI in the SUNXI ARMv7 port. There?s a bit of a quirk in how SUNXI handles DMA from what I understand of the patch. I think it may only affect the A10, but that?s outside the scope of this e-mail.
>
> The fix is a series of patches, but the first one is going to be the tough since the change nestled inside ahci_port_start() in sys/dev/ic/ahci.c. Here?s the web-view of the patch: (let me know if you need me to provide a diff for readability)
> https://github.com/bitrig/bitrig/commit/6342a27bfe4dc590f8e266e370f54846a766a737
>
> Now, I don?t care about the original author?s implementation choice, everyone is justified in their own actions. What I want to know is what needs to be done to increase the chances this will be committed by you folks.
>
> To be honest, I?d prefer to #ifdef the custom initialization based on target platform and clean up the ahci_softc.sc_flags bit field. Is everyone on board for that? Is there a better way?

The quirk bit should just be set in arch/armv7/sunxi/sxiahci.c
Change
sc->sc_flags |= AHCI_F_NO_PMP; /* XXX enough? */
to
sc->sc_flags |= AHCI_F_NO_PMP | AHCI_F_SUNXI_QUIRK;

That is how bitrig did it as well.

Some of the other register access changes in the sxiahci.c part of
https://github.com/bitrig/bitrig/commit/9859ab07da261ebb4f561ff2b5bc5802b63ac57c
may be of interest as well.

Reply | Threaded
Open this post in threaded view
|

Re: Proper way to implement platform specific quirk

Edwin Amsler
Okay, preferred method is the bitfield, not #ifdef.

I’ll get a patch done soon and submit.

On Nov 22, 2014, at 8:24 PM, Jonathan Gray <[hidden email]> wrote:

> On Sat, Nov 22, 2014 at 02:06:48PM -0600, Edwin Amsler wrote:
>> Hey there,
>>
>> This e-mail is about design and custom arch-specific code.
>>
>> I?m trying to author a patch from Bitrig to enable proper support for AHCI in the SUNXI ARMv7 port. There?s a bit of a quirk in how SUNXI handles DMA from what I understand of the patch. I think it may only affect the A10, but that?s outside the scope of this e-mail.
>>
>> The fix is a series of patches, but the first one is going to be the tough since the change nestled inside ahci_port_start() in sys/dev/ic/ahci.c. Here?s the web-view of the patch: (let me know if you need me to provide a diff for readability)
>> https://github.com/bitrig/bitrig/commit/6342a27bfe4dc590f8e266e370f54846a766a737
>>
>> Now, I don?t care about the original author?s implementation choice, everyone is justified in their own actions. What I want to know is what needs to be done to increase the chances this will be committed by you folks.
>>
>> To be honest, I?d prefer to #ifdef the custom initialization based on target platform and clean up the ahci_softc.sc_flags bit field. Is everyone on board for that? Is there a better way?
>
> The quirk bit should just be set in arch/armv7/sunxi/sxiahci.c
> Change
> sc->sc_flags |= AHCI_F_NO_PMP; /* XXX enough? */
> to
> sc->sc_flags |= AHCI_F_NO_PMP | AHCI_F_SUNXI_QUIRK;
>
> That is how bitrig did it as well.
>
> Some of the other register access changes in the sxiahci.c part of
> https://github.com/bitrig/bitrig/commit/9859ab07da261ebb4f561ff2b5bc5802b63ac57c
> may be of interest as well.


Reply | Threaded
Open this post in threaded view
|

Re: Proper way to implement platform specific quirk

Jonathan Gray-11
A bit flag, a bitfield is something else and should be avoided.

Ideally the values being written into the registers in sxiahci.c would
have their own defines instead of being a bunch of magic numbers as
well.  But that would be a seperate diff.

On Sat, Nov 22, 2014 at 10:04:06PM -0600, Edwin Amsler wrote:

> Okay, preferred method is the bitfield, not #ifdef.
>
> I?ll get a patch done soon and submit.
>
> On Nov 22, 2014, at 8:24 PM, Jonathan Gray <[hidden email]> wrote:
>
> > On Sat, Nov 22, 2014 at 02:06:48PM -0600, Edwin Amsler wrote:
> >> Hey there,
> >>
> >> This e-mail is about design and custom arch-specific code.
> >>
> >> I?m trying to author a patch from Bitrig to enable proper support for AHCI in the SUNXI ARMv7 port. There?s a bit of a quirk in how SUNXI handles DMA from what I understand of the patch. I think it may only affect the A10, but that?s outside the scope of this e-mail.
> >>
> >> The fix is a series of patches, but the first one is going to be the tough since the change nestled inside ahci_port_start() in sys/dev/ic/ahci.c. Here?s the web-view of the patch: (let me know if you need me to provide a diff for readability)
> >> https://github.com/bitrig/bitrig/commit/6342a27bfe4dc590f8e266e370f54846a766a737
> >>
> >> Now, I don?t care about the original author?s implementation choice, everyone is justified in their own actions. What I want to know is what needs to be done to increase the chances this will be committed by you folks.
> >>
> >> To be honest, I?d prefer to #ifdef the custom initialization based on target platform and clean up the ahci_softc.sc_flags bit field. Is everyone on board for that? Is there a better way?
> >
> > The quirk bit should just be set in arch/armv7/sunxi/sxiahci.c
> > Change
> > sc->sc_flags |= AHCI_F_NO_PMP; /* XXX enough? */
> > to
> > sc->sc_flags |= AHCI_F_NO_PMP | AHCI_F_SUNXI_QUIRK;
> >
> > That is how bitrig did it as well.
> >
> > Some of the other register access changes in the sxiahci.c part of
> > https://github.com/bitrig/bitrig/commit/9859ab07da261ebb4f561ff2b5bc5802b63ac57c
> > may be of interest as well.