All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: linux-sunxi@googlegroups.com, Chen-Yu Tsai <wens@csie.org>
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>,
	netdev@vger.kernel.org, Rob Herring <rob.herring@calxeda.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Maxime Ripard <maxime.ripard@free-electrons.com>
Subject: Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Date: Mon, 09 Dec 2013 17:16:52 +0100	[thread overview]
Message-ID: <52A5ECF4.6030301@redhat.com> (raw)
In-Reply-To: <52A5A52C.50605@st.com>

Hi,

On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
> Hi Chen,
> Good to know that Allwinner uses gmac.
>
> On ST SoC, we have very similar requirements, before we merge any of
> these changes I think we need to come up with common way to solve both
> Allwinner and ST SOCs use cases.
>
> I have already posted few patches on to net-dev
> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> driver.
>
>
> There are few reasons for the way I have done it.
>
> 1> I did not want to modify gmac driver in any sense to when a new SOC
> support is added.
> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> makes sense to represent it proper harware hierarchy.

On top often is not the correct term / how things are done in practice.

Most of the time the glue are modifications to the ip block, iow in
hardware they are not nested / hierarchical at all. We've had similar
constructs for ahci platform drivers and there we are actively trying to
move away from the whole nested platform devices as that has various
issues:

1) It is wrong / does not reflect reality
2) It breaks deferred probing which is often used on SOCs

I actually think Wens' approach using a SOC specific init function
in platform data is sound, and this is also used a lot else where.

As for using the nested approach elsewhere, I only know about AHCI
platform driver doing that, and there we are actively trying to move
away from it.


Now reading this has also made me take a closer look at wens' patch
for this. Wens, I see that you directly modify registers in the ccm
that is a big no-no instead you should add a helper function to
sunxi-clk.c and use that, see ie:
https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master


Giuseppe, I get the feeling the 0x148 registers st-gmac is using is
the same story, but I could be wrong. One important reason why accessing
regs like this directly is a big no no is because it is impossible to
do proper locking between multiple users, so 2 users doing read / modify / write
could end up racing. So accesses to such registers should always happen
through a single driver.


Regards,

Hans

WARNING: multiple messages have this Message-ID (diff)
From: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org,
	Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
Cc: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>,
	netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>,
	devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	Maxime Ripard
	<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Subject: Re: Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Date: Mon, 09 Dec 2013 17:16:52 +0100	[thread overview]
Message-ID: <52A5ECF4.6030301@redhat.com> (raw)
In-Reply-To: <52A5A52C.50605-qxv4g6HH51o@public.gmane.org>

Hi,

On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
> Hi Chen,
> Good to know that Allwinner uses gmac.
>
> On ST SoC, we have very similar requirements, before we merge any of
> these changes I think we need to come up with common way to solve both
> Allwinner and ST SOCs use cases.
>
> I have already posted few patches on to net-dev
> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> driver.
>
>
> There are few reasons for the way I have done it.
>
> 1> I did not want to modify gmac driver in any sense to when a new SOC
> support is added.
> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> makes sense to represent it proper harware hierarchy.

On top often is not the correct term / how things are done in practice.

Most of the time the glue are modifications to the ip block, iow in
hardware they are not nested / hierarchical at all. We've had similar
constructs for ahci platform drivers and there we are actively trying to
move away from the whole nested platform devices as that has various
issues:

1) It is wrong / does not reflect reality
2) It breaks deferred probing which is often used on SOCs

I actually think Wens' approach using a SOC specific init function
in platform data is sound, and this is also used a lot else where.

As for using the nested approach elsewhere, I only know about AHCI
platform driver doing that, and there we are actively trying to move
away from it.


Now reading this has also made me take a closer look at wens' patch
for this. Wens, I see that you directly modify registers in the ccm
that is a big no-no instead you should add a helper function to
sunxi-clk.c and use that, see ie:
https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master


Giuseppe, I get the feeling the 0x148 registers st-gmac is using is
the same story, but I could be wrong. One important reason why accessing
regs like this directly is a big no no is because it is impossible to
do proper locking between multiple users, so 2 users doing read / modify / write
could end up racing. So accesses to such registers should always happen
through a single driver.


Regards,

Hans

WARNING: multiple messages have this Message-ID (diff)
From: hdegoede@redhat.com (Hans de Goede)
To: linux-arm-kernel@lists.infradead.org
Subject: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Date: Mon, 09 Dec 2013 17:16:52 +0100	[thread overview]
Message-ID: <52A5ECF4.6030301@redhat.com> (raw)
In-Reply-To: <52A5A52C.50605@st.com>

Hi,

On 12/09/2013 12:10 PM, srinivas kandagatla wrote:
> Hi Chen,
> Good to know that Allwinner uses gmac.
>
> On ST SoC, we have very similar requirements, before we merge any of
> these changes I think we need to come up with common way to solve both
> Allwinner and ST SOCs use cases.
>
> I have already posted few patches on to net-dev
> http://lkml.org/lkml/2013/11/12/243 to add Glue driver on top of stmmac
> driver.
>
>
> There are few reasons for the way I have done it.
>
> 1> I did not want to modify gmac driver in any sense to when a new SOC
> support is added.
> 2> As the SOC specific glue logic sits on top of standard GMAC IP, it
> makes sense to represent it proper harware hierarchy.

On top often is not the correct term / how things are done in practice.

Most of the time the glue are modifications to the ip block, iow in
hardware they are not nested / hierarchical at all. We've had similar
constructs for ahci platform drivers and there we are actively trying to
move away from the whole nested platform devices as that has various
issues:

1) It is wrong / does not reflect reality
2) It breaks deferred probing which is often used on SOCs

I actually think Wens' approach using a SOC specific init function
in platform data is sound, and this is also used a lot else where.

As for using the nested approach elsewhere, I only know about AHCI
platform driver doing that, and there we are actively trying to move
away from it.


Now reading this has also made me take a closer look at wens' patch
for this. Wens, I see that you directly modify registers in the ccm
that is a big no-no instead you should add a helper function to
sunxi-clk.c and use that, see ie:
https://bitbucket.org/emiliolopez/linux/commits/2b95847d9aa4aa13317dd7358ffcbd951dcb5eff?at=master


Giuseppe, I get the feeling the 0x148 registers st-gmac is using is
the same story, but I could be wrong. One important reason why accessing
regs like this directly is a big no no is because it is impossible to
do proper locking between multiple users, so 2 users doing read / modify / write
could end up racing. So accesses to such registers should always happen
through a single driver.


Regards,

Hans

  reply	other threads:[~2013-12-09 16:17 UTC|newest]

Thread overview: 189+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-06 17:29 [PATCH 00/10] net: stmmac: Add sun7i GMAC glue layer Chen-Yu Tsai
2013-12-06 17:29 ` Chen-Yu Tsai
2013-12-06 17:29 ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 01/10] net: stmmac: Enable stmmac main clock when probing hardware Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-07 10:33   ` Maxime Ripard
2013-12-07 10:33     ` Maxime Ripard
2013-12-07 10:33     ` Maxime Ripard
2013-12-09  2:43     ` Chen-Yu Tsai
2013-12-09  2:43       ` Chen-Yu Tsai
2013-12-09  2:43       ` Chen-Yu Tsai
2013-12-09 10:09       ` [linux-sunxi] " Hans de Goede
2013-12-09 10:09         ` Hans de Goede
2013-12-09 10:09         ` Hans de Goede
2013-12-10 20:05       ` Maxime Ripard
2013-12-10 20:05         ` Maxime Ripard
2013-12-10 20:05         ` Maxime Ripard
2013-12-12  4:31         ` Chen-Yu Tsai
2013-12-12  4:31           ` Chen-Yu Tsai
2013-12-12  4:31           ` Chen-Yu Tsai
2013-12-09  7:14   ` Giuseppe CAVALLARO
2013-12-09  7:14     ` Giuseppe CAVALLARO
2013-12-09  7:14     ` Giuseppe CAVALLARO
2013-12-09  7:26     ` Chen-Yu Tsai
2013-12-09  7:26       ` Chen-Yu Tsai
2013-12-09  7:26       ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 02/10] net: stmmac: Honor DT parameter to force DMA store and forward mode Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 21:26   ` David Miller
2013-12-06 21:26     ` David Miller
     [not found]     ` <20131206.162606.2277176361893801778.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
2013-12-07  1:19       ` Chen-Yu Tsai
2013-12-07  1:23     ` Chen-Yu Tsai
2013-12-07  1:23       ` Chen-Yu Tsai
2013-12-07  1:23       ` Chen-Yu Tsai
2013-12-07 10:07       ` maxime.ripard
2013-12-07 10:07         ` maxime.ripard
2013-12-07 10:07         ` maxime.ripard
2013-12-07 11:06         ` Tomasz Figa
2013-12-07 11:06           ` Tomasz Figa
2013-12-07 11:06           ` Tomasz Figa
2013-12-09  2:59           ` [linux-sunxi] " Chen-Yu Tsai
2013-12-09  2:59             ` Chen-Yu Tsai
2013-12-09  2:59             ` Chen-Yu Tsai
2013-12-10 20:10             ` [linux-sunxi] " maxime.ripard
2013-12-10 20:10               ` maxime.ripard
2013-12-10 20:10               ` maxime.ripard
2013-12-06 17:29 ` [PATCH 03/10] net: stmmac: Use platform data tied with compatible strings Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 21:26   ` David Miller
2013-12-06 21:26     ` David Miller
2013-12-07  2:13     ` Chen-Yu Tsai
2013-12-07  2:13       ` Chen-Yu Tsai
2013-12-07  2:13       ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-07 10:27   ` Maxime Ripard
2013-12-07 10:27     ` Maxime Ripard
2013-12-07 10:27     ` Maxime Ripard
2013-12-07 11:12     ` Tomasz Figa
2013-12-07 11:12       ` Tomasz Figa
2013-12-07 11:12       ` Tomasz Figa
2013-12-07 11:46       ` Maxime Ripard
2013-12-07 11:46         ` Maxime Ripard
2013-12-07 11:46         ` Maxime Ripard
2013-12-07 12:50         ` Tomasz Figa
2013-12-07 12:50           ` Tomasz Figa
2013-12-07 12:50           ` Tomasz Figa
2013-12-07 13:34           ` [linux-sunxi] " Emilio López
2013-12-07 13:34             ` Emilio López
2013-12-07 13:34             ` Emilio López
2013-12-09 11:10   ` srinivas kandagatla
2013-12-09 11:10     ` srinivas kandagatla
2013-12-09 11:10     ` srinivas kandagatla
2013-12-09 16:16     ` Hans de Goede [this message]
2013-12-09 16:16       ` [linux-sunxi] " Hans de Goede
2013-12-09 16:16       ` Hans de Goede
2013-12-09 17:56       ` [linux-sunxi] " Chen-Yu Tsai
2013-12-09 17:56         ` Chen-Yu Tsai
2013-12-09 17:56         ` Chen-Yu Tsai
2013-12-09 19:04         ` [linux-sunxi] " Hans de Goede
2013-12-09 19:04           ` Hans de Goede
2013-12-09 19:04           ` Hans de Goede
2013-12-10 20:14           ` [linux-sunxi] " Maxime Ripard
2013-12-10 20:14             ` Maxime Ripard
2013-12-10 20:14             ` Maxime Ripard
2013-12-09 17:34     ` Chen-Yu Tsai
2013-12-09 17:34       ` Chen-Yu Tsai
2013-12-09 17:34       ` Chen-Yu Tsai
2013-12-10 14:59       ` srinivas kandagatla
2013-12-10 14:59         ` srinivas kandagatla
2013-12-10 14:59         ` srinivas kandagatla
2013-12-10 20:23         ` Maxime Ripard
2013-12-10 20:23           ` Maxime Ripard
2013-12-10 20:23           ` Maxime Ripard
2013-12-11 12:17         ` Chen-Yu Tsai
2013-12-11 12:17           ` Chen-Yu Tsai
2013-12-11 12:17           ` Chen-Yu Tsai
2013-12-11 14:45           ` srinivas kandagatla
2013-12-11 14:45             ` srinivas kandagatla
2013-12-11 14:45             ` srinivas kandagatla
2013-12-12  7:27             ` Chen-Yu Tsai
2013-12-12  7:27               ` Chen-Yu Tsai
2013-12-12  7:27               ` Chen-Yu Tsai
2013-12-12  9:04             ` Maxime Ripard
2013-12-12  9:04               ` Maxime Ripard
2013-12-12  9:04               ` Maxime Ripard
2013-12-12 10:31               ` Chen-Yu Tsai
2013-12-12 10:31                 ` Chen-Yu Tsai
2013-12-12 10:31                 ` Chen-Yu Tsai
2013-12-13 10:38                 ` Maxime Ripard
2013-12-13 10:38                   ` Maxime Ripard
2013-12-13 10:38                   ` Maxime Ripard
2013-12-24  3:27                   ` [linux-sunxi] " Chen-Yu Tsai
2013-12-24  3:27                     ` Chen-Yu Tsai
2013-12-24  3:27                     ` Chen-Yu Tsai
2014-01-02 13:11                     ` [linux-sunxi] " srinivas kandagatla
2014-01-02 13:11                       ` srinivas kandagatla
2014-01-02 13:11                       ` srinivas kandagatla
2014-01-07 10:24                       ` [linux-sunxi] " Chen-Yu Tsai
2014-01-07 10:24                         ` Chen-Yu Tsai
2014-01-07 10:24                         ` Chen-Yu Tsai
2013-12-09 11:21   ` srinivas kandagatla
2013-12-09 11:21     ` srinivas kandagatla
2013-12-09 11:21     ` srinivas kandagatla
2013-12-09 13:44     ` Sergei Shtylyov
2013-12-09 13:44       ` Sergei Shtylyov
2013-12-09 13:44       ` Sergei Shtylyov
2013-12-09 15:45       ` [linux-sunxi] " Chen-Yu Tsai
2013-12-09 15:45         ` Chen-Yu Tsai
2013-12-09 15:45         ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 05/10] ARM: dts: sun7i: Add GMAC controller node to sun7i DTSI Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 06/10] ARM: dts: sun7i: Add pin muxing options for the GMAC Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 07/10] ARM: dts: sun7i: cubietruck: Enable " Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 21:09   ` Florian Fainelli
2013-12-06 21:09     ` Florian Fainelli
2013-12-06 21:09     ` Florian Fainelli
2013-12-07  1:35     ` Chen-Yu Tsai
2013-12-07  1:35       ` Chen-Yu Tsai
2013-12-07  1:35       ` Chen-Yu Tsai
2013-12-07  1:57       ` Florian Fainelli
2013-12-07  1:57         ` Florian Fainelli
2013-12-07  1:57         ` Florian Fainelli
2013-12-09  2:55         ` Chen-Yu Tsai
2013-12-09  2:55           ` Chen-Yu Tsai
2013-12-09  2:55           ` Chen-Yu Tsai
2013-12-09 17:48           ` Florian Fainelli
2013-12-09 17:48             ` Florian Fainelli
2013-12-09 17:48             ` Florian Fainelli
2013-12-10  4:11             ` Chen-Yu Tsai
2013-12-10  4:11               ` Chen-Yu Tsai
2013-12-10  4:11               ` Chen-Yu Tsai
2013-12-10 17:23               ` Florian Fainelli
2013-12-10 17:23                 ` Florian Fainelli
2013-12-10 17:23                 ` Florian Fainelli
2013-12-13 10:21         ` Giuseppe CAVALLARO
2013-12-13 10:21           ` Giuseppe CAVALLARO
2013-12-13 10:21           ` Giuseppe CAVALLARO
2013-12-13 10:21           ` Giuseppe CAVALLARO
2013-12-06 17:29 ` [PATCH 08/10] ARM: dts: sun7i: cubieboard2: Enable GMAC instead of EMAC Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 21:10   ` Florian Fainelli
2013-12-06 21:10     ` Florian Fainelli
2013-12-06 21:10     ` Florian Fainelli
2013-12-06 17:29 ` [PATCH 09/10] ARM: dts: sun7i: olinuxino-micro: " Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29 ` [PATCH 10/10] ARM: dts: sun7i: Add ethernet alias for GMAC Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-06 17:29   ` Chen-Yu Tsai
2013-12-07 10:15   ` Maxime Ripard
2013-12-07 10:15     ` Maxime Ripard
2013-12-07 10:15     ` Maxime Ripard
2013-12-07 16:20     ` Chen-Yu Tsai
2013-12-07 16:20       ` Chen-Yu Tsai
2013-12-07 16:20       ` Chen-Yu Tsai
2013-12-06 20:52 ` [linux-sunxi] [PATCH 00/10] net: stmmac: Add sun7i GMAC glue layer Michal Suchanek
2013-12-06 20:52   ` Michal Suchanek
2013-12-06 20:52   ` Michal Suchanek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52A5ECF4.6030301@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=maxime.ripard@free-electrons.com \
    --cc=netdev@vger.kernel.org \
    --cc=peppe.cavallaro@st.com \
    --cc=rob.herring@calxeda.com \
    --cc=wens@csie.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.