Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340486 > unrolled thread

Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for  stm32 chip Alexandre Torgue <alexandre.torgue@gmail.com> - 2016-02-23 11:00 +0100
    Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for  stm32 chip Joachim Eastwood <manabian@gmail.com> - 2016-02-23 12:30 +0100
      Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for  stm32 chip Alexandre Torgue <alexandre.torgue@gmail.com> - 2016-02-23 14:20 +0100

#1340486 — Re: [PATCH 1/4] net: ethernet: dwmac: add Ethernet glue logic for stm32 chip

2016-02-22 22:52 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> On 22 February 2016 at 15:50, Alexandre Torgue
> <alexandre.torgue@gmail.com> wrote:
>> 2016-02-13 14:48 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
>>> On 3 February 2016 at 15:54, Alexandre TORGUE
>>> <alexandre.torgue@gmail.com> wrote:
>>>> +       plat_dat->bsp_priv = dwmac;
>>>> +       plat_dat->init = stm32_dwmac_init;
>>>> +       plat_dat->exit = stm32_dwmac_exit;
>>>
>>> Instead of using these callbacks could you rather implement the PM
>>> callbacks directly in this driver?
>>> I don't think it should add much code and it will make it look more
>>> like standard driver. This will also give you some more control and
>>> flexibility in your code.
>>
>> I prefer to keep the code as it is. Glue layer is directly linked to
>> stmmac driver and I don't want to brake the link between the glue and
>> the stmmac driver.
>
> What do you mean by break the link?
>

I thought that you wanted to split stmmac_pltfr_supend (glue part and
stmamc part), but I well understood it is not the case (sorry for
mistake).

> There has been numerous of patch sets to make the stmmac "glue"
> drivers into more standard platform drivers.
> http://marc.info/?l=linux-netdev&m=143159850631093&w=2
> http://marc.info/?l=linux-netdev&m=143708560009851&w=2
> http://marc.info/?l=linux-netdev&m=143812136600541&w=2
>
> Do you see any advantage by using the init and exit hooks in your
> driver instead of using the standard driver PM callbacks and remove
> function?
> The only "cost" I see is slightly more boilerplate code. But since you
> already have init/exit functions you could easily make them into PM
> resume/suspend so I doubt there would be much increase in code size.
>

If I well understood you want to continue the stmmac glue driver
rework by moving stmmac_pltfr_suspend/resume/remove in each glue
driver (stm32, sun, sti ....).
Each glue driver will call directly stmmac_suspend/resume/remove and
their own init/exit function.
If it is what you meant, I can do it.

> One other thing;
> Do you need to have the PHY mode setup code in the init function which
> is called each time on resume?

I can't guarantee that after a suspend the sysconfig register will
contain same data than before suspend.

Best regards.

Alex

> If you could move it to probe you could drop the interface priv data
> member and use plat_dat->interface as stmmac_probe_config_dt() has
> already done of_get_phy_mode().
>
>
> regards,
> Joachim Eastwood

[toc] | [next] | [standalone]

#1340556

On 23 February 2016 at 10:59, Alexandre Torgue
<alexandre.torgue@gmail.com> wrote:
> 2016-02-22 22:52 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
>> On 22 February 2016 at 15:50, Alexandre Torgue
>> <alexandre.torgue@gmail.com> wrote:
>>> 2016-02-13 14:48 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
>>>> On 3 February 2016 at 15:54, Alexandre TORGUE
>>>> <alexandre.torgue@gmail.com> wrote:
>>>>> +       plat_dat->bsp_priv = dwmac;
>>>>> +       plat_dat->init = stm32_dwmac_init;
>>>>> +       plat_dat->exit = stm32_dwmac_exit;
>>>>
>>>> Instead of using these callbacks could you rather implement the PM
>>>> callbacks directly in this driver?
>>>> I don't think it should add much code and it will make it look more
>>>> like standard driver. This will also give you some more control and
>>>> flexibility in your code.
>>>
>>> I prefer to keep the code as it is. Glue layer is directly linked to
>>> stmmac driver and I don't want to brake the link between the glue and
>>> the stmmac driver.
>>
>> What do you mean by break the link?
>>
>
> I thought that you wanted to split stmmac_pltfr_supend (glue part and
> stmamc part), but I well understood it is not the case (sorry for
> mistake).
>
>> There has been numerous of patch sets to make the stmmac "glue"
>> drivers into more standard platform drivers.
>> http://marc.info/?l=linux-netdev&m=143159850631093&w=2
>> http://marc.info/?l=linux-netdev&m=143708560009851&w=2
>> http://marc.info/?l=linux-netdev&m=143812136600541&w=2
>>
>> Do you see any advantage by using the init and exit hooks in your
>> driver instead of using the standard driver PM callbacks and remove
>> function?
>> The only "cost" I see is slightly more boilerplate code. But since you
>> already have init/exit functions you could easily make them into PM
>> resume/suspend so I doubt there would be much increase in code size.
>>
>
> If I well understood you want to continue the stmmac glue driver
> rework by moving stmmac_pltfr_suspend/resume/remove in each glue
> driver (stm32, sun, sti ....).

At least I want to avoid the init/exit callbacks for new drivers like
stm32-dwmac.


> Each glue driver will call directly stmmac_suspend/resume/remove and
> their own init/exit function.
> If it is what you meant, I can do it.

Yes, in your stm32 driver's suspend/resume/remove functions call
stmmac_suspend/stmmac_resume/stmmac_dvr_remove directly. Then you
shouldn't need to use the init/exit callbacks. Just put the need code
in the driver's suspend/resume/remove functions instead of init/exit
functions.

For example:
static int stm32_dwmac_resume(struct device *dev)
{
    struct net_device *ndev = dev_get_drvdata(dev);
    struct plat_stmmacenet_data *plat_dat = get_stmmac_plat_data(ndev)
    struct stm32_dwmac *dwmac =plat_dat->bsp_priv;

    /* enable clocks */
    /* set phy mode */

    return stmmac_resume(ndev);
}

If it makes sense to have the enable clk/phy mode stuff in it's own
function that is fine too.


>> One other thing;
>> Do you need to have the PHY mode setup code in the init function which
>> is called each time on resume?
>
> I can't guarantee that after a suspend the sysconfig register will
> contain same data than before suspend.

I see.


regards,
Joachim Eastwood

[toc] | [prev] | [next] | [standalone]

#1340603

2016-02-23 12:21 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
> On 23 February 2016 at 10:59, Alexandre Torgue
> <alexandre.torgue@gmail.com> wrote:
>> 2016-02-22 22:52 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
>>> On 22 February 2016 at 15:50, Alexandre Torgue
>>> <alexandre.torgue@gmail.com> wrote:
>>>> 2016-02-13 14:48 GMT+01:00 Joachim  Eastwood <manabian@gmail.com>:
>>>>> On 3 February 2016 at 15:54, Alexandre TORGUE
>>>>> <alexandre.torgue@gmail.com> wrote:
>>>>>> +       plat_dat->bsp_priv = dwmac;
>>>>>> +       plat_dat->init = stm32_dwmac_init;
>>>>>> +       plat_dat->exit = stm32_dwmac_exit;
>>>>>
>>>>> Instead of using these callbacks could you rather implement the PM
>>>>> callbacks directly in this driver?
>>>>> I don't think it should add much code and it will make it look more
>>>>> like standard driver. This will also give you some more control and
>>>>> flexibility in your code.
>>>>
>>>> I prefer to keep the code as it is. Glue layer is directly linked to
>>>> stmmac driver and I don't want to brake the link between the glue and
>>>> the stmmac driver.
>>>
>>> What do you mean by break the link?
>>>
>>
>> I thought that you wanted to split stmmac_pltfr_supend (glue part and
>> stmamc part), but I well understood it is not the case (sorry for
>> mistake).
>>
>>> There has been numerous of patch sets to make the stmmac "glue"
>>> drivers into more standard platform drivers.
>>> http://marc.info/?l=linux-netdev&m=143159850631093&w=2
>>> http://marc.info/?l=linux-netdev&m=143708560009851&w=2
>>> http://marc.info/?l=linux-netdev&m=143812136600541&w=2
>>>
>>> Do you see any advantage by using the init and exit hooks in your
>>> driver instead of using the standard driver PM callbacks and remove
>>> function?
>>> The only "cost" I see is slightly more boilerplate code. But since you
>>> already have init/exit functions you could easily make them into PM
>>> resume/suspend so I doubt there would be much increase in code size.
>>>
>>
>> If I well understood you want to continue the stmmac glue driver
>> rework by moving stmmac_pltfr_suspend/resume/remove in each glue
>> driver (stm32, sun, sti ....).
>
> At least I want to avoid the init/exit callbacks for new drivers like
> stm32-dwmac.
>
>
>> Each glue driver will call directly stmmac_suspend/resume/remove and
>> their own init/exit function.
>> If it is what you meant, I can do it.
>
> Yes, in your stm32 driver's suspend/resume/remove functions call
> stmmac_suspend/stmmac_resume/stmmac_dvr_remove directly. Then you
> shouldn't need to use the init/exit callbacks. Just put the need code
> in the driver's suspend/resume/remove functions instead of init/exit
> functions.
>
> For example:
> static int stm32_dwmac_resume(struct device *dev)
> {
>     struct net_device *ndev = dev_get_drvdata(dev);
>     struct plat_stmmacenet_data *plat_dat = get_stmmac_plat_data(ndev)
>     struct stm32_dwmac *dwmac =plat_dat->bsp_priv;
>
>     /* enable clocks */
>     /* set phy mode */
>
>     return stmmac_resume(ndev);
> }
>
> If it makes sense to have the enable clk/phy mode stuff in it's own
> function that is fine too.

Ok. I will send v2 with this approach. After this series I could
provide a new series to change all glues with this approach ? (but I
will not able to test on each platform).

Best regards

Alexandre
>
>
>>> One other thing;
>>> Do you need to have the PHY mode setup code in the init function which
>>> is called each time on resume?
>>
>> I can't guarantee that after a suspend the sysconfig register will
>> contain same data than before suspend.
>
> I see.
>
>
> regards,
> Joachim Eastwood

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web