RFC on STM32 Ethernet driver


Erwin Rol
 

Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


Erwan Gouriou
 

Hi Erwin,


Thanks for proposing this driver, I think it will interest a lot of people.

About your last point, let me mention ST view on this:

This is a question of trade-off. Using HAL has some drawbacks (some might
be objectives like footprint or more subjective like CamelCase), but it's
definitely possible in Zephyr.
Main interest is that it will help having a reliable driver working on all STM32
SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
stm32f7xx under review) faster. More supported SoCs also means more
interested people, which will help in getting the driver mature.
In the end, it should save time to work on end applications rather than porting.

A native driver, specific to the IP (which might also be used on other SoCs),
might be more optimized, but this might also take you some time before
having it mature for complex IPs such as ethernet (or USB..).

In any case, I think it would be great to have your driver upstreamed in
a first time. Then, time will tell if a native driver is nice to have or required.

Erwan



On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...> wrote:
Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@lists.zephyrproject.org
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel


Erwin Rol
 

Hey Erwan,

like you say some things are subjective when it comes to HAL's, but
somethings really bother me with the STM32 HAL.

The most important is how to deal with IRQ's. In Zephyr I have to setup
the ISR like this;

IRQ_CONNECT(ETH_IRQn, CONFIG_ETH_STM32_IRQ_PRI, eth_isr,
DEVICE_GET(eth0_stm32), 0);
irq_enable(ETH_IRQn);

Now the eth_isr will be called on IRQ. But to use everything from the
HAL we must call the;

void HAL_ETH_IRQHandler(ETH_HandleTypeDef *heth)

That function will than call;

__weak void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)

But as you can see we register a ISR to take a "struct eth0_stm32*" but
all the HAL functions only have "ETH_HandleTypeDef *heth" arguments.

So our driver ISR somehow needs something to go from ETH_HandleTypeDef
*heth to struct eth0_stm32*. The other way around is simple, the
eth0_stm32 struct just has a ETH_HandleTypeDef member.

I solved this to copy the HAL_ETH_IRQHandler handler to my driver and
change it to take my eth0_stm32 struct as argument.

But I really don't like the idea to use half the HAL and copy the other
half.

Would there be a better solution?

BTW: this applies to pretty much everything the uses STM32 IRQ's. I
already noticed the the UART does not use the HAL, but that would have
had the same problem.

It would have been nice if those ETH_HandleTypeDef structures had a
void* user_data member.

That directly brings me to the next point, how are things managed with
upstream (ST) if Zephyr finds problems/bugs in the HAL? Bugs they
probably would not mind to fix, but how to deal with things that are
Zephyr specific, will that mean the Zephyr STM32 HAL will diverge from
the official ST HAL ?

- Erwin

On 6-6-2017 11:19, Erwan Gouriou wrote:
Hi Erwin,


Thanks for proposing this driver, I think it will interest a lot of people.

About your last point, let me mention ST view on this:

This is a question of trade-off. Using HAL has some drawbacks (some might
be objectives like footprint or more subjective like CamelCase), but it's
definitely possible in Zephyr.
Main interest is that it will help having a reliable driver working on
all STM32
SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
stm32f7xx under review) faster. More supported SoCs also means more
interested people, which will help in getting the driver mature.
In the end, it should save time to work on end applications rather than
porting.

A native driver, specific to the IP (which might also be used on other
SoCs),
might be more optimized, but this might also take you some time before
having it mature for complex IPs such as ethernet (or USB..).

In any case, I think it would be great to have your driver upstreamed in
a first time. Then, time will tell if a native driver is nice to have or
required.

Erwan



On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...
<mailto:mailinglists@...>> wrote:

Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
<https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54>
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
<mailto:Zephyr-devel@...>
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
<https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel>


Neil Armstrong
 

Hi Erwin,

You can use CONTAINER_OF() to get the parent structure like
https://github.com/superna9999/zephyr/blob/stm32f4_i2c/drivers/i2c/i2c_stm32.c#L93

Neil

On 06/09/2017 12:22 PM, Erwin Rol wrote:
Hey Erwan,

like you say some things are subjective when it comes to HAL's, but
somethings really bother me with the STM32 HAL.

The most important is how to deal with IRQ's. In Zephyr I have to setup
the ISR like this;

IRQ_CONNECT(ETH_IRQn, CONFIG_ETH_STM32_IRQ_PRI, eth_isr,
DEVICE_GET(eth0_stm32), 0);
irq_enable(ETH_IRQn);

Now the eth_isr will be called on IRQ. But to use everything from the
HAL we must call the;

void HAL_ETH_IRQHandler(ETH_HandleTypeDef *heth)

That function will than call;

__weak void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)

But as you can see we register a ISR to take a "struct eth0_stm32*" but
all the HAL functions only have "ETH_HandleTypeDef *heth" arguments.

So our driver ISR somehow needs something to go from ETH_HandleTypeDef
*heth to struct eth0_stm32*. The other way around is simple, the
eth0_stm32 struct just has a ETH_HandleTypeDef member.

I solved this to copy the HAL_ETH_IRQHandler handler to my driver and
change it to take my eth0_stm32 struct as argument.

But I really don't like the idea to use half the HAL and copy the other
half.

Would there be a better solution?

BTW: this applies to pretty much everything the uses STM32 IRQ's. I
already noticed the the UART does not use the HAL, but that would have
had the same problem.

It would have been nice if those ETH_HandleTypeDef structures had a
void* user_data member.

That directly brings me to the next point, how are things managed with
upstream (ST) if Zephyr finds problems/bugs in the HAL? Bugs they
probably would not mind to fix, but how to deal with things that are
Zephyr specific, will that mean the Zephyr STM32 HAL will diverge from
the official ST HAL ?

- Erwin

On 6-6-2017 11:19, Erwan Gouriou wrote:
Hi Erwin,


Thanks for proposing this driver, I think it will interest a lot of people.

About your last point, let me mention ST view on this:

This is a question of trade-off. Using HAL has some drawbacks (some might
be objectives like footprint or more subjective like CamelCase), but it's
definitely possible in Zephyr.
Main interest is that it will help having a reliable driver working on
all STM32
SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
stm32f7xx under review) faster. More supported SoCs also means more
interested people, which will help in getting the driver mature.
In the end, it should save time to work on end applications rather than
porting.

A native driver, specific to the IP (which might also be used on other
SoCs),
might be more optimized, but this might also take you some time before
having it mature for complex IPs such as ethernet (or USB..).

In any case, I think it would be great to have your driver upstreamed in
a first time. Then, time will tell if a native driver is nice to have or
required.

Erwan



On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...
<mailto:mailinglists@...>> wrote:

Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
<https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54>
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
<mailto:Zephyr-devel@...>
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
<https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel>

_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel


Erwin Rol
 

Hey Neil,

Thanks, somehow just didn't think about that. Will rewrite the eth
driver to use the CONTAINER_OF() construct.

BTW is that i2c driver going to be included in mainline? :-)

- Erwin

On 9-6-2017 13:25, Neil Armstrong wrote:
Hi Erwin,

You can use CONTAINER_OF() to get the parent structure like
https://github.com/superna9999/zephyr/blob/stm32f4_i2c/drivers/i2c/i2c_stm32.c#L93

Neil

On 06/09/2017 12:22 PM, Erwin Rol wrote:
Hey Erwan,

like you say some things are subjective when it comes to HAL's, but
somethings really bother me with the STM32 HAL.

The most important is how to deal with IRQ's. In Zephyr I have to setup
the ISR like this;

IRQ_CONNECT(ETH_IRQn, CONFIG_ETH_STM32_IRQ_PRI, eth_isr,
DEVICE_GET(eth0_stm32), 0);
irq_enable(ETH_IRQn);

Now the eth_isr will be called on IRQ. But to use everything from the
HAL we must call the;

void HAL_ETH_IRQHandler(ETH_HandleTypeDef *heth)

That function will than call;

__weak void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)

But as you can see we register a ISR to take a "struct eth0_stm32*" but
all the HAL functions only have "ETH_HandleTypeDef *heth" arguments.

So our driver ISR somehow needs something to go from ETH_HandleTypeDef
*heth to struct eth0_stm32*. The other way around is simple, the
eth0_stm32 struct just has a ETH_HandleTypeDef member.

I solved this to copy the HAL_ETH_IRQHandler handler to my driver and
change it to take my eth0_stm32 struct as argument.

But I really don't like the idea to use half the HAL and copy the other
half.

Would there be a better solution?

BTW: this applies to pretty much everything the uses STM32 IRQ's. I
already noticed the the UART does not use the HAL, but that would have
had the same problem.

It would have been nice if those ETH_HandleTypeDef structures had a
void* user_data member.

That directly brings me to the next point, how are things managed with
upstream (ST) if Zephyr finds problems/bugs in the HAL? Bugs they
probably would not mind to fix, but how to deal with things that are
Zephyr specific, will that mean the Zephyr STM32 HAL will diverge from
the official ST HAL ?

- Erwin

On 6-6-2017 11:19, Erwan Gouriou wrote:
Hi Erwin,


Thanks for proposing this driver, I think it will interest a lot of people.

About your last point, let me mention ST view on this:

This is a question of trade-off. Using HAL has some drawbacks (some might
be objectives like footprint or more subjective like CamelCase), but it's
definitely possible in Zephyr.
Main interest is that it will help having a reliable driver working on
all STM32
SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
stm32f7xx under review) faster. More supported SoCs also means more
interested people, which will help in getting the driver mature.
In the end, it should save time to work on end applications rather than
porting.

A native driver, specific to the IP (which might also be used on other
SoCs),
might be more optimized, but this might also take you some time before
having it mature for complex IPs such as ethernet (or USB..).

In any case, I think it would be great to have your driver upstreamed in
a first time. Then, time will tell if a native driver is nice to have or
required.

Erwan



On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...
<mailto:mailinglists@...>> wrote:

Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
<https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54>
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
<mailto:Zephyr-devel@...>
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
<https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel>

_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel


Erwan Gouriou
 

Hi Erwin,


As mentioned by Neil, CONTAINER_OF is a good solution that is used in other projects using STM32Cube.

About your last point:
There won't be any branching from STM32Cube for Zephyr.
Generic bugs (I mean not Zephyr specific) should be raised and solved into official STM32Cube (don't hesitate to contact me for that purpose).
When it comes to adaptation to Zephyr, it could be possible if it is identified that STM32Cube solution is not fitting for generic adaptation
(when same flaw is reported in other projects like mBed for instance), but it is unlikely to get zephyr specific modification.

About I2C driver, Jorge is working on a LL based version that should go upstream (work in progress).

Cheers
Erwan


On 9 June 2017 at 13:38, Erwin Rol <mailinglists@...> wrote:
Hey Neil,

Thanks, somehow just didn't think about that. Will rewrite the eth
driver to use the CONTAINER_OF() construct.

BTW is that i2c driver going to be included in mainline? :-)

- Erwin


On 9-6-2017 13:25, Neil Armstrong wrote:
> Hi Erwin,
>
> You can use CONTAINER_OF() to get the parent structure like
> https://github.com/superna9999/zephyr/blob/stm32f4_i2c/drivers/i2c/i2c_stm32.c#L93
>
> Neil
>
> On 06/09/2017 12:22 PM, Erwin Rol wrote:
>> Hey Erwan,
>>
>> like you say some things are subjective when it comes to HAL's, but
>> somethings really bother me with the STM32 HAL.
>>
>> The most important is how to deal with IRQ's. In Zephyr I have to setup
>> the ISR like this;
>>
>> IRQ_CONNECT(ETH_IRQn, CONFIG_ETH_STM32_IRQ_PRI, eth_isr,
>>              DEVICE_GET(eth0_stm32), 0);
>> irq_enable(ETH_IRQn);
>>
>> Now the eth_isr will be called on IRQ. But to use everything from the
>> HAL we must call the;
>>
>> void HAL_ETH_IRQHandler(ETH_HandleTypeDef *heth)
>>
>> That function will than call;
>>
>> __weak void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)
>>
>> But as you can see we register a ISR to take a "struct eth0_stm32*" but
>> all the HAL functions only have "ETH_HandleTypeDef *heth"  arguments.
>>
>> So our driver ISR somehow needs something to go from ETH_HandleTypeDef
>> *heth to struct eth0_stm32*. The other way around is simple, the
>> eth0_stm32 struct just has a ETH_HandleTypeDef member.
>>
>> I solved this to copy the HAL_ETH_IRQHandler handler to my driver and
>> change it to take my eth0_stm32 struct as argument.
>>
>> But I really don't like the idea to use half the HAL and copy the other
>> half.
>>
>> Would there be a better solution?
>>
>> BTW: this applies to pretty much everything the uses STM32 IRQ's. I
>> already noticed the the UART does not use the HAL, but that would have
>> had the same problem.
>>
>> It would have been nice if those  ETH_HandleTypeDef structures had a
>> void* user_data member.
>>
>> That directly brings me to the next point, how are things managed with
>> upstream (ST) if Zephyr finds problems/bugs in the HAL? Bugs they
>> probably would not mind to fix, but how to deal with things that are
>> Zephyr specific, will that mean the Zephyr STM32 HAL will diverge from
>> the official ST HAL ?
>>
>> - Erwin
>>
>> On 6-6-2017 11:19, Erwan Gouriou wrote:
>>> Hi Erwin,
>>>
>>>
>>> Thanks for proposing this driver, I think it will interest a lot of people.
>>>
>>> About your last point, let me mention ST view on this:
>>>
>>> This is a question of trade-off. Using HAL has some drawbacks (some might
>>> be objectives like footprint or more subjective like CamelCase), but it's
>>> definitely possible in Zephyr.
>>> Main interest is that it will help having a reliable driver working on
>>> all STM32
>>> SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
>>> stm32f7xx under review) faster. More supported SoCs also means more
>>> interested people, which will help in getting the driver mature.
>>> In the end, it should save time to work on end applications rather than
>>> porting.
>>>
>>> A native driver, specific to the IP (which might also be used on other
>>> SoCs),
>>> might be more optimized, but this might also take you some time before
>>> having it mature for complex IPs such as ethernet (or USB..).
>>>
>>> In any case, I think it would be great to have your driver upstreamed in
>>> a first time. Then, time will tell if a native driver is nice to have or
>>> required.
>>>
>>> Erwan
>>>
>>>
>>>
>>> On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...
>>> <mailto:mailinglists@erwinrol.com>> wrote:
>>>
>>>     Hello,
>>>
>>>     I made an initial STM32F4 Ethernet driver. It is based on the HAL
>>>     version, and doesn't try to be smart with DMA and packet buffer memory,
>>>     it just does memcpy to/from the DMA buffers. But it works.
>>>
>>>     It can be found at github;
>>>
>>>     https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
>>>     <https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54>
>>>     Since it is for the olimex_stm32_e407 board (which is still pending to
>>>     be merged) I didn't want to create a pull request yet.
>>>
>>>     The driver use the STM32 HAL (or at least parts of it), and as I
>>>     mentioned "it works" but I am very uncertain about if I like the HAL
>>>     thing or not.
>>>
>>>     For example the ISR uses this __weak callbacks, but you will not have
>>>     access to your device structure anymore, and it is very unclear what is
>>>     happening. So I pretty much copied the whole ISR to only have the
>>>     possibility to access my network device structure passed to the ISR.
>>>
>>>     With the use of macros like "ETH" naming collisions are bound to happen
>>>     sooner or later.
>>>
>>>     The CamelCase naming convention of the HAL just gives me a headache, but
>>>     that's a matter of taste I guess.
>>>
>>>     I know I am opening a can of worms, but I really could use some guidance
>>>     on if using the HAL for more complex drivers like Ethernet is the way to
>>>     go or if just making a clean implementation is better.
>>>
>>>     - Erwin
>>>
>>>
>>>     _______________________________________________
>>>     Zephyr-devel mailing list
>>>     Zephyr-devel@lists.zephyrproject.org
>>>     <mailto:Zephyr-devel@lists.zephyrproject.org>
>>>     https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
>>>     <https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel>
>>>
>>>
>>
>> _______________________________________________
>> Zephyr-devel mailing list
>> Zephyr-devel@lists.zephyrproject.org
>> https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
>>
>



Erwin Rol
 

Hey Erwan,

On 9-6-2017 14:17, Erwan Gouriou wrote:
Hi Erwin,


As mentioned by Neil, CONTAINER_OF is a good solution that is used in
other projects using STM32Cube.
Yes, I will rewrite it to use CONTRAINER_OF.


About I2C driver, Jorge is working on a LL based version that should go
upstream (work in progress).
So the I2C driver is being rewritten even though there is a working
version (I assume Neil's HAL version is working)? Now you are making me
even more unsure about the HAL, because it sounds like using the HAL
isn't really that wanted, and drivers will be rewritten using LL when
ever possible ?

- Erwin


Erwan Gouriou
 

Hi Erwin,


LL based version was initiated without knowing Neil had a HAL based driver,
and from what I am aware of (unless I missed this information), Neil never
proposed to upstream it. I let him comment (or not) on the status of his driver.

Then, I confirm the strategy about bringing LL driver when possible and 
use HAL when there is a significant gain: on complex drivers (Eth and USB today).
Then, once HAL based ethernet driver will be available, functional cross series,
and mature, I don't think one will attempt to port it to LL anytime soon,
(Besides, there is no LL for these drivers, and I don't know if there will ever be*).

Finally, let me mention that some part of UART driver uses HAL today.
This was done earlier before having a clear view on STM32Cube matching
vs Zephyr. It should be ported totally to LL when possible, as per strategy.


* you'll find a file stm32l4xx_ll_usb.c, but is it actually a low layer of the HAL driver,
I admit this is confusing.

On 9 June 2017 at 15:04, Erwin Rol <mailinglists@...> wrote:
Hey Erwan,

On 9-6-2017 14:17, Erwan Gouriou wrote:
> Hi Erwin,
>
>
> As mentioned by Neil, CONTAINER_OF is a good solution that is used in
> other projects using STM32Cube.

Yes, I will rewrite it to use CONTRAINER_OF.

>
> About I2C driver, Jorge is working on a LL based version that should go
> upstream (work in progress).

So the I2C driver is being rewritten even though there is a working
version (I assume Neil's HAL version is working)? Now you are making me
even more unsure about the HAL, because it sounds like using the HAL
isn't really that wanted, and drivers will be rewritten using LL when
ever possible ?

- Erwin


Neil Armstrong
 

On 06/09/2017 03:42 PM, Erwan Gouriou wrote:
Hi Erwin,


LL based version was initiated without knowing Neil had a HAL based driver,
and from what I am aware of (unless I missed this information), Neil never
proposed to upstream it. I let him comment (or not) on the status of his driver.
Indeed, for the little story I was trying to test the sensor (to upstream support for it) on
the stm32f4discovery board, but when I hoocked up everything (driver, ...) to test the driver,
I realized the sensor was connected to SPI only pins....... so I stopped here.

But Jorde (ldts) made it work to use it as base for the LL based i2c driver, base on the
already upstream stm32lx driver.


Then, I confirm the strategy about bringing LL driver when possible and
use HAL when there is a significant gain: on complex drivers (Eth and USB today).
Then, once HAL based ethernet driver will be available, functional cross series,
and mature, I don't think one will attempt to port it to LL anytime soon,
(Besides, there is no LL for these drivers, and I don't know if there will ever be*).
And it's when the strategy became clear I definitely stopped thinking about the HAL based i2c driver.

However I started a FMC HAL based driver because for the above reasons, the LL version
would have no real sense since it's quite a complex stuff.


Finally, let me mention that some part of UART driver uses HAL today.
This was done earlier before having a clear view on STM32Cube matching
vs Zephyr. It should be ported totally to LL when possible, as per strategy.
Neil


* you'll find a file stm32l4xx_ll_usb.c, but is it actually a low layer of the HAL driver,
I admit this is confusing.

On 9 June 2017 at 15:04, Erwin Rol <mailinglists@... <mailto:mailinglists@...>> wrote:

Hey Erwan,

On 9-6-2017 14:17, Erwan Gouriou wrote:
> Hi Erwin,
>
>
> As mentioned by Neil, CONTAINER_OF is a good solution that is used in
> other projects using STM32Cube.

Yes, I will rewrite it to use CONTRAINER_OF.

>
> About I2C driver, Jorge is working on a LL based version that should go
> upstream (work in progress).

So the I2C driver is being rewritten even though there is a working
version (I assume Neil's HAL version is working)? Now you are making me
even more unsure about the HAL, because it sounds like using the HAL
isn't really that wanted, and drivers will be rewritten using LL when
ever possible ?

- Erwin


Erwin Rol
 

Hey Neil, Erwan,

thanks for the infos. I guess the confusions are largely because Zephyr
is still such a young project. Under Linux there are so many drivers to
look at as examples and if 10 use HAL and 90 use LL one would probably
assume it would be better to use LL. But in zephyr there are like 3
STM32 drivers? and about as many ethernet drivers, that makes it hard
for someone new to the project (like me) to figure out what is the way
to go.

Even though it might be a bit annoying for you guys, I really do
appreciate your feedback.

So for now I will cleanup my HAL based ETH driver and we will see what
the future brings.

One thing that crossed my mind, would it be an idea to use a driver name
with "hal" in the name, for example eth_stm32_hal ? That way at a later
stage a second parallel version could be made without needing to throw
away the HAL version.

- Erwin

On 9-6-2017 15:48, Neil Armstrong wrote:
On 06/09/2017 03:42 PM, Erwan Gouriou wrote:
Hi Erwin,


LL based version was initiated without knowing Neil had a HAL based driver,
and from what I am aware of (unless I missed this information), Neil never
proposed to upstream it. I let him comment (or not) on the status of his driver.
Indeed, for the little story I was trying to test the sensor (to upstream support for it) on
the stm32f4discovery board, but when I hoocked up everything (driver, ...) to test the driver,
I realized the sensor was connected to SPI only pins....... so I stopped here.

But Jorde (ldts) made it work to use it as base for the LL based i2c driver, base on the
already upstream stm32lx driver.


Then, I confirm the strategy about bringing LL driver when possible and
use HAL when there is a significant gain: on complex drivers (Eth and USB today).
Then, once HAL based ethernet driver will be available, functional cross series,
and mature, I don't think one will attempt to port it to LL anytime soon,
(Besides, there is no LL for these drivers, and I don't know if there will ever be*).
And it's when the strategy became clear I definitely stopped thinking about the HAL based i2c driver.

However I started a FMC HAL based driver because for the above reasons, the LL version
would have no real sense since it's quite a complex stuff.


Finally, let me mention that some part of UART driver uses HAL today.
This was done earlier before having a clear view on STM32Cube matching
vs Zephyr. It should be ported totally to LL when possible, as per strategy.
Neil


* you'll find a file stm32l4xx_ll_usb.c, but is it actually a low layer of the HAL driver,
I admit this is confusing.

On 9 June 2017 at 15:04, Erwin Rol <mailinglists@... <mailto:mailinglists@...>> wrote:

Hey Erwan,

On 9-6-2017 14:17, Erwan Gouriou wrote:
> Hi Erwin,
>
>
> As mentioned by Neil, CONTAINER_OF is a good solution that is used in
> other projects using STM32Cube.

Yes, I will rewrite it to use CONTRAINER_OF.

>
> About I2C driver, Jorge is working on a LL based version that should go
> upstream (work in progress).

So the I2C driver is being rewritten even though there is a working
version (I assume Neil's HAL version is working)? Now you are making me
even more unsure about the HAL, because it sounds like using the HAL
isn't really that wanted, and drivers will be rewritten using LL when
ever possible ?

- Erwin


Erwan Gouriou
 


On 9 June 2017 at 15:59, Erwin Rol <mailinglists@...> wrote:
Hey Neil, Erwan,

thanks for the infos. I guess the confusions are largely because Zephyr
is still such a young project. Under Linux there are so many drivers to
look at as examples and if 10 use HAL and 90 use LL one would probably
assume it would be better to use LL. But in zephyr there are like 3
STM32 drivers? and about as many ethernet drivers, that makes it hard
for someone new to the project (like me) to figure out what is the way
to go.

Even though it might be a bit annoying for you guys, I really do
appreciate your feedback.

No problem, I understand it is not easy to jump in. And agreed we 
might miss some material to make all this clear.
 
So for now I will cleanup my HAL based ETH driver and we will see what
the future brings.

Great! We look forward to see it upstreamed
 
One thing that crossed my mind, would it be an idea to use a driver name
with "hal" in the name, for example eth_stm32_hal ? That way at a later
stage a second parallel version could be made without needing to throw
away the HAL version.

Seems right!
  
- Erwin


On 9-6-2017 15:48, Neil Armstrong wrote:
> On 06/09/2017 03:42 PM, Erwan Gouriou wrote:
>> Hi Erwin,
>>
>>
>> LL based version was initiated without knowing Neil had a HAL based driver,
>> and from what I am aware of (unless I missed this information), Neil never
>> proposed to upstream it. I let him comment (or not) on the status of his driver.
>
> Indeed, for the little story I was trying to test the sensor (to upstream support for it) on
> the stm32f4discovery board, but when I hoocked up everything (driver, ...) to test the driver,
> I realized the sensor was connected to SPI only pins....... so I stopped here.
>
> But Jorde (ldts) made it work to use it as base for the LL based i2c driver, base on the
> already upstream stm32lx driver.
>
>>
>> Then, I confirm the strategy about bringing LL driver when possible and
>> use HAL when there is a significant gain: on complex drivers (Eth and USB today).
>> Then, once HAL based ethernet driver will be available, functional cross series,
>> and mature, I don't think one will attempt to port it to LL anytime soon,
>> (Besides, there is no LL for these drivers, and I don't know if there will ever be*).
>
> And it's when the strategy became clear I definitely stopped thinking about the HAL based i2c driver.
>
> However I started a FMC HAL based driver because for the above reasons, the LL version
> would have no real sense since it's quite a complex stuff.
>
>>
>> Finally, let me mention that some part of UART driver uses HAL today.
>> This was done earlier before having a clear view on STM32Cube matching
>> vs Zephyr. It should be ported totally to LL when possible, as per strategy.
>
> Neil
>>
>>
>> * you'll find a file stm32l4xx_ll_usb.c, but is it actually a low layer of the HAL driver,
>> I admit this is confusing.
>>
>> On 9 June 2017 at 15:04, Erwin Rol <mailinglists@... <mailto:mailinglists@erwinrol.com>> wrote:
>>
>>     Hey Erwan,
>>
>>     On 9-6-2017 14:17, Erwan Gouriou wrote:
>>     > Hi Erwin,
>>     >
>>     >
>>     > As mentioned by Neil, CONTAINER_OF is a good solution that is used in
>>     > other projects using STM32Cube.
>>
>>     Yes, I will rewrite it to use CONTRAINER_OF.
>>
>>     >
>>     > About I2C driver, Jorge is working on a LL based version that should go
>>     > upstream (work in progress).
>>
>>     So the I2C driver is being rewritten even though there is a working
>>     version (I assume Neil's HAL version is working)? Now you are making me
>>     even more unsure about the HAL, because it sounds like using the HAL
>>     isn't really that wanted, and drivers will be rewritten using LL when
>>     ever possible ?
>>
>>     - Erwin
>>
>>
>



Erwin Rol
 

Hey All,

I rewrote it by using CONTAINER_OF() and it works fine and is lot
cleaner. Tanks for the hint.

- Erwin

On 9-6-2017 13:25, Neil Armstrong wrote:
Hi Erwin,

You can use CONTAINER_OF() to get the parent structure like
https://github.com/superna9999/zephyr/blob/stm32f4_i2c/drivers/i2c/i2c_stm32.c#L93

Neil

On 06/09/2017 12:22 PM, Erwin Rol wrote:
Hey Erwan,

like you say some things are subjective when it comes to HAL's, but
somethings really bother me with the STM32 HAL.

The most important is how to deal with IRQ's. In Zephyr I have to setup
the ISR like this;

IRQ_CONNECT(ETH_IRQn, CONFIG_ETH_STM32_IRQ_PRI, eth_isr,
DEVICE_GET(eth0_stm32), 0);
irq_enable(ETH_IRQn);

Now the eth_isr will be called on IRQ. But to use everything from the
HAL we must call the;

void HAL_ETH_IRQHandler(ETH_HandleTypeDef *heth)

That function will than call;

__weak void HAL_ETH_RxCpltCallback(ETH_HandleTypeDef *heth)

But as you can see we register a ISR to take a "struct eth0_stm32*" but
all the HAL functions only have "ETH_HandleTypeDef *heth" arguments.

So our driver ISR somehow needs something to go from ETH_HandleTypeDef
*heth to struct eth0_stm32*. The other way around is simple, the
eth0_stm32 struct just has a ETH_HandleTypeDef member.

I solved this to copy the HAL_ETH_IRQHandler handler to my driver and
change it to take my eth0_stm32 struct as argument.

But I really don't like the idea to use half the HAL and copy the other
half.

Would there be a better solution?

BTW: this applies to pretty much everything the uses STM32 IRQ's. I
already noticed the the UART does not use the HAL, but that would have
had the same problem.

It would have been nice if those ETH_HandleTypeDef structures had a
void* user_data member.

That directly brings me to the next point, how are things managed with
upstream (ST) if Zephyr finds problems/bugs in the HAL? Bugs they
probably would not mind to fix, but how to deal with things that are
Zephyr specific, will that mean the Zephyr STM32 HAL will diverge from
the official ST HAL ?

- Erwin

On 6-6-2017 11:19, Erwan Gouriou wrote:
Hi Erwin,


Thanks for proposing this driver, I think it will interest a lot of people.

About your last point, let me mention ST view on this:

This is a question of trade-off. Using HAL has some drawbacks (some might
be objectives like footprint or more subjective like CamelCase), but it's
definitely possible in Zephyr.
Main interest is that it will help having a reliable driver working on
all STM32
SoCs supporting ethernet (today in zephyr: stm32f407, stm32f429, stm32f469,
stm32f7xx under review) faster. More supported SoCs also means more
interested people, which will help in getting the driver mature.
In the end, it should save time to work on end applications rather than
porting.

A native driver, specific to the IP (which might also be used on other
SoCs),
might be more optimized, but this might also take you some time before
having it mature for complex IPs such as ethernet (or USB..).

In any case, I think it would be great to have your driver upstreamed in
a first time. Then, time will tell if a native driver is nice to have or
required.

Erwan



On 3 June 2017 at 15:17, Erwin Rol <mailinglists@...
<mailto:mailinglists@...>> wrote:

Hello,

I made an initial STM32F4 Ethernet driver. It is based on the HAL
version, and doesn't try to be smart with DMA and packet buffer memory,
it just does memcpy to/from the DMA buffers. But it works.

It can be found at github;

https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54
<https://github.com/lowlander/zephyr/commit/6a49b0e0c435f0529b97255c675fb57e54d23a54>
Since it is for the olimex_stm32_e407 board (which is still pending to
be merged) I didn't want to create a pull request yet.

The driver use the STM32 HAL (or at least parts of it), and as I
mentioned "it works" but I am very uncertain about if I like the HAL
thing or not.

For example the ISR uses this __weak callbacks, but you will not have
access to your device structure anymore, and it is very unclear what is
happening. So I pretty much copied the whole ISR to only have the
possibility to access my network device structure passed to the ISR.

With the use of macros like "ETH" naming collisions are bound to happen
sooner or later.

The CamelCase naming convention of the HAL just gives me a headache, but
that's a matter of taste I guess.

I know I am opening a can of worms, but I really could use some guidance
on if using the HAL for more complex drivers like Ethernet is the way to
go or if just making a clean implementation is better.

- Erwin


_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
<mailto:Zephyr-devel@...>
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel
<https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel>

_______________________________________________
Zephyr-devel mailing list
Zephyr-devel@...
https://lists.zephyrproject.org/mailman/listinfo/zephyr-devel