Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Thu, Jan 2, 2014 at 9:11 PM, srinivas kandagatla wrote: > Hi Chen, > > On 24/12/13 03:27, Chen-Yu Tsai wrote: >> Srinivas, >> >> Let's keep platform data as of_data, so SoC compatibles can pass >> hardware feature flags for cores that don't support auto-detection. > > I understand your concern here, But making platform_data as of_data > would just open a wide door for other hacky stuff too. So other glue > drivers would just use this interface instead, ignoring standard > interfaces. Also there would be issues on who takes the priority if the > parameters are specified in multiple places. > > > Can't this field/s be one of the variable in the struct stmmac_of_data > rather than reusing platform data? We (sunxi) need .has_gmac and .tx_coe. To be generic and usable to other SoCs, it seems like I would be adding most of the fields from platform data. But maybe future glue layers will only need the callbacks. I can not be sure. Also since snps,phy-addr should be deprecated, I am thinking of changing the default phy address to auto-detect, until Giuseppe merges his phy node support work. I will post a v2 series this week. >> >> We can adjust the callbacks as you suggested, and I added a .free >> to complement .setup. Driver private data and platform data are >> off limits to the callbacks. My version of the callbacks: >> >> void (*fix_mac_speed)(void *priv, unsigned int speed); >> void (*bus_setup)(void __iomem *ioaddr); > > In reply to your question in last email: > bus_setup this is something very specific to ST which configures the bus > parameters of AMBA-to-STBUS converter. This register falls inside the > memory map of stmmac. I see. IMHO it could be merged into .init? > >> void *(*setup)(struct platform_device *pdev); >> void (*free)(struct platform_device *pdev, void *priv); >> int (*init)(struct platform_device *pdev, void *priv); >> void (*exit)(struct platform_device *pdev, void *priv); >> > > The callbacks to Ok to me, unless Peppe has more comments on this. Ok. Cheers ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Thu, Jan 2, 2014 at 9:11 PM, srinivas kandagatla srinivas.kandaga...@st.com wrote: Hi Chen, On 24/12/13 03:27, Chen-Yu Tsai wrote: Srinivas, Let's keep platform data as of_data, so SoC compatibles can pass hardware feature flags for cores that don't support auto-detection. I understand your concern here, But making platform_data as of_data would just open a wide door for other hacky stuff too. So other glue drivers would just use this interface instead, ignoring standard interfaces. Also there would be issues on who takes the priority if the parameters are specified in multiple places. Can't this field/s be one of the variable in the struct stmmac_of_data rather than reusing platform data? We (sunxi) need .has_gmac and .tx_coe. To be generic and usable to other SoCs, it seems like I would be adding most of the fields from platform data. But maybe future glue layers will only need the callbacks. I can not be sure. Also since snps,phy-addr should be deprecated, I am thinking of changing the default phy address to auto-detect, until Giuseppe merges his phy node support work. I will post a v2 series this week. We can adjust the callbacks as you suggested, and I added a .free to complement .setup. Driver private data and platform data are off limits to the callbacks. My version of the callbacks: void (*fix_mac_speed)(void *priv, unsigned int speed); void (*bus_setup)(void __iomem *ioaddr); In reply to your question in last email: bus_setup this is something very specific to ST which configures the bus parameters of AMBA-to-STBUS converter. This register falls inside the memory map of stmmac. I see. IMHO it could be merged into .init? void *(*setup)(struct platform_device *pdev); void (*free)(struct platform_device *pdev, void *priv); int (*init)(struct platform_device *pdev, void *priv); void (*exit)(struct platform_device *pdev, void *priv); The callbacks to Ok to me, unless Peppe has more comments on this. Ok. Cheers ChenYu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi Chen, On 24/12/13 03:27, Chen-Yu Tsai wrote: > Srinivas, > > Let's keep platform data as of_data, so SoC compatibles can pass > hardware feature flags for cores that don't support auto-detection. I understand your concern here, But making platform_data as of_data would just open a wide door for other hacky stuff too. So other glue drivers would just use this interface instead, ignoring standard interfaces. Also there would be issues on who takes the priority if the parameters are specified in multiple places. Can't this field/s be one of the variable in the struct stmmac_of_data rather than reusing platform data? > > We can adjust the callbacks as you suggested, and I added a .free > to complement .setup. Driver private data and platform data are > off limits to the callbacks. My version of the callbacks: > > void (*fix_mac_speed)(void *priv, unsigned int speed); > void (*bus_setup)(void __iomem *ioaddr); In reply to your question in last email: bus_setup this is something very specific to ST which configures the bus parameters of AMBA-to-STBUS converter. This register falls inside the memory map of stmmac. > void *(*setup)(struct platform_device *pdev); > void (*free)(struct platform_device *pdev, void *priv); > int (*init)(struct platform_device *pdev, void *priv); > void (*exit)(struct platform_device *pdev, void *priv); > The callbacks to Ok to me, unless Peppe has more comments on this. Thanks, srini > > Cheers, > > ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi Chen, On 24/12/13 03:27, Chen-Yu Tsai wrote: Srinivas, Let's keep platform data as of_data, so SoC compatibles can pass hardware feature flags for cores that don't support auto-detection. I understand your concern here, But making platform_data as of_data would just open a wide door for other hacky stuff too. So other glue drivers would just use this interface instead, ignoring standard interfaces. Also there would be issues on who takes the priority if the parameters are specified in multiple places. Can't this field/s be one of the variable in the struct stmmac_of_data rather than reusing platform data? We can adjust the callbacks as you suggested, and I added a .free to complement .setup. Driver private data and platform data are off limits to the callbacks. My version of the callbacks: void (*fix_mac_speed)(void *priv, unsigned int speed); void (*bus_setup)(void __iomem *ioaddr); In reply to your question in last email: bus_setup this is something very specific to ST which configures the bus parameters of AMBA-to-STBUS converter. This register falls inside the memory map of stmmac. void *(*setup)(struct platform_device *pdev); void (*free)(struct platform_device *pdev, void *priv); int (*init)(struct platform_device *pdev, void *priv); void (*exit)(struct platform_device *pdev, void *priv); The callbacks to Ok to me, unless Peppe has more comments on this. Thanks, srini Cheers, ChenYu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Fri, Dec 13, 2013 at 6:38 PM, Maxime Ripard wrote: > On Thu, Dec 12, 2013 at 06:31:43PM +0800, Chen-Yu Tsai wrote: >> Hi, >> >> On Thu, Dec 12, 2013 at 5:04 PM, Maxime Ripard >> wrote: >> > Hi, >> > >> > On Wed, Dec 11, 2013 at 02:45:08PM +, srinivas kandagatla wrote: >> >> >>> 1. .tx_coe >> >> >>>This is not exported in the DT bindings. >> >> >>>Looking at stmmac code, not setting this seems to disable all >> >> >>>checksum offloading. >> >> >> >> >> >> Why cant this go via DT as well? >> >> > >> >> > If you and Giuseppe are OK with this, why not? >> >> Am Ok with it. >> > >> > Please note that I'm opposed to this until someone explain why putting >> > it in the DT is relevant (and not just convenient). >> >> Checksum offloading is an optional feature[1], implemented starting >> from version 3.20a. It is not tied to a specific IP version. As such, >> using a "snps,dwmac-" compatible isn't a good fit here. > > No, but we're not in such case. Since we have a compatible of our own, > we can derive it from that. Putting a property in the DT would only be > redundant. > >> stmmac does auto-detection for optional features on MAC version > 3.50a. >> This is what Srinivas was referring to. >> >> Unfortunately, our MAC is < 3.50a. No auto-detection. We could add a >> "snps,dwmac-tx-coe" compatible for this, or the seperate DT property. >> >> The other way would be to pass the flags in the initial .data with the >> SoC specific compatible. Other SoCs with the same feature won't be >> able to reuse the same compatible though. > > Which is already pretty much the case, since we have to deal with > Allwinner specific code and features. > > A new compatible is cheap to maintain, a new property is not. Srinivas, Let's keep platform data as of_data, so SoC compatibles can pass hardware feature flags for cores that don't support auto-detection. We can adjust the callbacks as you suggested, and I added a .free to complement .setup. Driver private data and platform data are off limits to the callbacks. My version of the callbacks: void (*fix_mac_speed)(void *priv, unsigned int speed); void (*bus_setup)(void __iomem *ioaddr); void *(*setup)(struct platform_device *pdev); void (*free)(struct platform_device *pdev, void *priv); int (*init)(struct platform_device *pdev, void *priv); void (*exit)(struct platform_device *pdev, void *priv); Cheers, ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Fri, Dec 13, 2013 at 6:38 PM, Maxime Ripard maxime.rip...@free-electrons.com wrote: On Thu, Dec 12, 2013 at 06:31:43PM +0800, Chen-Yu Tsai wrote: Hi, On Thu, Dec 12, 2013 at 5:04 PM, Maxime Ripard maxime.rip...@free-electrons.com wrote: Hi, On Wed, Dec 11, 2013 at 02:45:08PM +, srinivas kandagatla wrote: 1. .tx_coe This is not exported in the DT bindings. Looking at stmmac code, not setting this seems to disable all checksum offloading. Why cant this go via DT as well? If you and Giuseppe are OK with this, why not? Am Ok with it. Please note that I'm opposed to this until someone explain why putting it in the DT is relevant (and not just convenient). Checksum offloading is an optional feature[1], implemented starting from version 3.20a. It is not tied to a specific IP version. As such, using a snps,dwmac-version compatible isn't a good fit here. No, but we're not in such case. Since we have a compatible of our own, we can derive it from that. Putting a property in the DT would only be redundant. stmmac does auto-detection for optional features on MAC version 3.50a. This is what Srinivas was referring to. Unfortunately, our MAC is 3.50a. No auto-detection. We could add a snps,dwmac-tx-coe compatible for this, or the seperate DT property. The other way would be to pass the flags in the initial .data with the SoC specific compatible. Other SoCs with the same feature won't be able to reuse the same compatible though. Which is already pretty much the case, since we have to deal with Allwinner specific code and features. A new compatible is cheap to maintain, a new property is not. Srinivas, Let's keep platform data as of_data, so SoC compatibles can pass hardware feature flags for cores that don't support auto-detection. We can adjust the callbacks as you suggested, and I added a .free to complement .setup. Driver private data and platform data are off limits to the callbacks. My version of the callbacks: void (*fix_mac_speed)(void *priv, unsigned int speed); void (*bus_setup)(void __iomem *ioaddr); void *(*setup)(struct platform_device *pdev); void (*free)(struct platform_device *pdev, void *priv); int (*init)(struct platform_device *pdev, void *priv); void (*exit)(struct platform_device *pdev, void *priv); Cheers, ChenYu -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
On Mon, Dec 09, 2013 at 08:04:21PM +0100, Hans de Goede wrote: > >>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 > > > >Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or > >"gmac interface type bit" has been bugging me. > > > >Additionally, the TX clock has 2 inputs (not counting MII [1]). > >The internal one is most likely controlled by the GMAC. The clock > >rate is set internally to match the link speed. The external clock > >source has controllable dividers to get the correct clock rate. > >This shouldn't be hard to model with CCF though. > > > >In hardware, this is probably a mux between the GMAC clock generator > >and the GMAC data transmit logic. > > > >My current plan is to choose MII when the clock is disabled, > >and choose either of the inputs when it is enabled. I will > >have to learn more about the CCF first. > > OK, in this case I would be tempted to just go with a custom sunxi > function in the sunx-clk mode like what we have for the mmc stuff, > but if you think you can model this with the regular clock stuff, > that is of course fine too :) Note that it's also ok to implement the clock driver out of drivers/clk if it makes more sense. So, if it's more convenient for you to declare both drivers (clock and gmac), say in the glue file, I'm totally fine whith that. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
On Mon, Dec 09, 2013 at 08:04:21PM +0100, Hans de Goede wrote: 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 Yes, this has been raised by Maxime. The odd GMAC_IF_TYPE_RGMII or gmac interface type bit has been bugging me. Additionally, the TX clock has 2 inputs (not counting MII [1]). The internal one is most likely controlled by the GMAC. The clock rate is set internally to match the link speed. The external clock source has controllable dividers to get the correct clock rate. This shouldn't be hard to model with CCF though. In hardware, this is probably a mux between the GMAC clock generator and the GMAC data transmit logic. My current plan is to choose MII when the clock is disabled, and choose either of the inputs when it is enabled. I will have to learn more about the CCF first. OK, in this case I would be tempted to just go with a custom sunxi function in the sunx-clk mode like what we have for the mmc stuff, but if you think you can model this with the regular clock stuff, that is of course fine too :) Note that it's also ok to implement the clock driver out of drivers/clk if it makes more sense. So, if it's more convenient for you to declare both drivers (clock and gmac), say in the glue file, I'm totally fine whith that. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On 12/09/2013 06:56 PM, Chen-Yu Tsai wrote: Hi, On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede wrote: 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 Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or "gmac interface type bit" has been bugging me. Additionally, the TX clock has 2 inputs (not counting MII [1]). The internal one is most likely controlled by the GMAC. The clock rate is set internally to match the link speed. The external clock source has controllable dividers to get the correct clock rate. This shouldn't be hard to model with CCF though. In hardware, this is probably a mux between the GMAC clock generator and the GMAC data transmit logic. My current plan is to choose MII when the clock is disabled, and choose either of the inputs when it is enabled. I will have to learn more about the CCF first. OK, in this case I would be tempted to just go with a custom sunxi function in the sunx-clk mode like what we have for the mmc stuff, but if you think you can model this with the regular clock stuff, that is of course fine too :) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede wrote: > 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 Yes, this has been raised by Maxime. The odd "GMAC_IF_TYPE_RGMII" or "gmac interface type bit" has been bugging me. Additionally, the TX clock has 2 inputs (not counting MII [1]). The internal one is most likely controlled by the GMAC. The clock rate is set internally to match the link speed. The external clock source has controllable dividers to get the correct clock rate. This shouldn't be hard to model with CCF though. In hardware, this is probably a mux between the GMAC clock generator and the GMAC data transmit logic. My current plan is to choose MII when the clock is disabled, and choose either of the inputs when it is enabled. I will have to learn more about the CCF first. [1] In MII mode, the TX clock is sent by the PHY to the MAC. From the MAC's viewpoint, it seems like a clock source. However, from the PHY's viewpoint, it is the provider. In RGMII mode, the MAC provides the TX clock to the PHY, and times the TX data lines to the clock. So in a way, the PHY is the true consumer of the TX clock. ChenYu > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi,
On Mon, Dec 9, 2013 at 9:44 PM, Sergei Shtylyov
wrote:
> Hello.
>
>
> On 09-12-2013 15:21, srinivas kandagatla wrote:
>
>>> +static int sun7i_gmac_init(struct platform_device *pdev)
>>> +{
>>> + struct resource *res;
>>> + struct device *dev = >dev;
>>> + void __iomem *addr = NULL;
>>> + struct plat_stmmacenet_data *plat_dat = NULL;
>
>
>No need to initialize it since you're assigning to it right below.
>
>
>>> + u32 priv_clk_reg;
>>> +
>>> + plat_dat = dev_get_platdata(>dev);
>>> + if (!plat_dat)
>>> + return -EINVAL;
>
>
>> dev_get_platdata will return NULL for DT, So this function will fail all
>> the time.
>
>
>Indeed, unless the probe() method assigns it from the 'data' field of
> 'struct of_device_id'.
>
>
>> How is it supposed to work?
>> Am I missing some thing?
>
>
>Look at stmmac_pltfr_probe().
Actually, Srinivas is right.
The platform data assigned from the 'data field' is not the same one as
dev_get_platdata().
This is something I missed when I was reworking the patches.
ChenYu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi,
On Mon, Dec 9, 2013 at 9:44 PM, Sergei Shtylyov
sergei.shtyl...@cogentembedded.com wrote:
Hello.
On 09-12-2013 15:21, srinivas kandagatla wrote:
+static int sun7i_gmac_init(struct platform_device *pdev)
+{
+ struct resource *res;
+ struct device *dev = pdev-dev;
+ void __iomem *addr = NULL;
+ struct plat_stmmacenet_data *plat_dat = NULL;
No need to initialize it since you're assigning to it right below.
+ u32 priv_clk_reg;
+
+ plat_dat = dev_get_platdata(pdev-dev);
+ if (!plat_dat)
+ return -EINVAL;
dev_get_platdata will return NULL for DT, So this function will fail all
the time.
Indeed, unless the probe() method assigns it from the 'data' field of
'struct of_device_id'.
How is it supposed to work?
Am I missing some thing?
Look at stmmac_pltfr_probe().
Actually, Srinivas is right.
The platform data assigned from the 'data field' is not the same one as
dev_get_platdata().
This is something I missed when I was reworking the patches.
ChenYu
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede hdego...@redhat.com wrote: 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 Yes, this has been raised by Maxime. The odd GMAC_IF_TYPE_RGMII or gmac interface type bit has been bugging me. Additionally, the TX clock has 2 inputs (not counting MII [1]). The internal one is most likely controlled by the GMAC. The clock rate is set internally to match the link speed. The external clock source has controllable dividers to get the correct clock rate. This shouldn't be hard to model with CCF though. In hardware, this is probably a mux between the GMAC clock generator and the GMAC data transmit logic. My current plan is to choose MII when the clock is disabled, and choose either of the inputs when it is enabled. I will have to learn more about the CCF first. [1] In MII mode, the TX clock is sent by the PHY to the MAC. From the MAC's viewpoint, it seems like a clock source. However, from the PHY's viewpoint, it is the provider. In RGMII mode, the MAC provides the TX clock to the PHY, and times the TX data lines to the clock. So in a way, the PHY is the true consumer of the TX clock. ChenYu 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
Hi, On 12/09/2013 06:56 PM, Chen-Yu Tsai wrote: Hi, On Tue, Dec 10, 2013 at 12:16 AM, Hans de Goede hdego...@redhat.com wrote: 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 Yes, this has been raised by Maxime. The odd GMAC_IF_TYPE_RGMII or gmac interface type bit has been bugging me. Additionally, the TX clock has 2 inputs (not counting MII [1]). The internal one is most likely controlled by the GMAC. The clock rate is set internally to match the link speed. The external clock source has controllable dividers to get the correct clock rate. This shouldn't be hard to model with CCF though. In hardware, this is probably a mux between the GMAC clock generator and the GMAC data transmit logic. My current plan is to choose MII when the clock is disabled, and choose either of the inputs when it is enabled. I will have to learn more about the CCF first. OK, in this case I would be tempted to just go with a custom sunxi function in the sunx-clk mode like what we have for the mmc stuff, but if you think you can model this with the regular clock stuff, that is of course fine too :) Regards, Hans -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
El 07/12/13 09:50, Tomasz Figa escribió:
On Saturday 07 of December 2013 12:46:16 Maxime Ripard wrote:
On Sat, Dec 07, 2013 at 12:12:26PM +0100, Tomasz Figa wrote:
On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
Chen-Yu, Mike,
On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
The Allwinner A20 has an ethernet controller that seems to be
an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
which is supported by the stmmac driver.
Allwinner's GMAC requires setting additional registers in the SoC's
clock control unit.
The exact version of the DWMAC IP that Allwinner uses is unknown,
thus the exact feature set is unknown.
Signed-off-by: Chen-Yu Tsai
---
.../bindings/net/allwinner,sun7i-gmac.txt | 22 +++
drivers/net/ethernet/stmicro/stmmac/Kconfig| 12
drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 76 ++
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +
.../net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 +
6 files changed, 117 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
new file mode 100644
index 000..271554a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
@@ -0,0 +1,22 @@
+* Allwinner GMAC ethernet controller
+
+This device is a platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+Required properties:
+ - compatible: Should be "allwinner,sun7i-gmac"
Please use sun7i-a20-gmac here.
+ - reg: Address and length of register set for the device and corresponding
+ clock control
+Examples:
+
+ gmac: ethernet@01c5 {
+ compatible = "allwinner,sun7i-gmac";
+ reg = <0x01c5 0x1>,
+ <0x01c20164 0x4>;
This is actually a clock, and should probably be registered in the
common clock framework.
Mike: This small register actually is a regular muxer/divider, except
that it has some bits that are of interest to the ethernet controller
(for example to set wether it's using GMII or RGMII to communicate
with the phy), that, as far as I'm aware of, aren't really fitting
into the CCF.
Do you have some recommendation on how to proceed?
Maybe make a thin "real" clock driver in this hardware glue, that
provides !exported function to set this *GMII thing.
Is this register part of a bigger IP block that manages clocks for other
IP blocks than stmmac as well? If not, I don't see a point of exporting
a clock from inside of the GMAC "domain" just to feed it back into it
as the only user.
This register is actually part of the SoC clock controller. So it sits
right beside the other clocks registers controlling the clocks of the
other devices, and is not part of the GMAC IP itself.
Is there any description for GMAC_IF_TYPE_RGMII and GMAC_TX_CLK fields?
Name of the latter sounds like a normal clock mux, but the former is
just a mystery (especially why it is a part of the clock controller).
You can find the register documented on page 89, table 1.5.4.65 of
http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-sunxi] Re: [PATCH 04/10] net: stmmac: sunxi platfrom extensions for GMAC in Allwinner A20 SoC's
El 07/12/13 09:50, Tomasz Figa escribió:
On Saturday 07 of December 2013 12:46:16 Maxime Ripard wrote:
On Sat, Dec 07, 2013 at 12:12:26PM +0100, Tomasz Figa wrote:
On Saturday 07 of December 2013 11:27:10 Maxime Ripard wrote:
Chen-Yu, Mike,
On Sat, Dec 07, 2013 at 01:29:37AM +0800, Chen-Yu Tsai wrote:
The Allwinner A20 has an ethernet controller that seems to be
an early version of Synopsys DesignWare MAC 10/100/1000 Universal,
which is supported by the stmmac driver.
Allwinner's GMAC requires setting additional registers in the SoC's
clock control unit.
The exact version of the DWMAC IP that Allwinner uses is unknown,
thus the exact feature set is unknown.
Signed-off-by: Chen-Yu Tsai w...@csie.org
---
.../bindings/net/allwinner,sun7i-gmac.txt | 22 +++
drivers/net/ethernet/stmicro/stmmac/Kconfig| 12
drivers/net/ethernet/stmicro/stmmac/Makefile | 1 +
drivers/net/ethernet/stmicro/stmmac/dwmac-sunxi.c | 76 ++
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 3 +
.../net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 +
6 files changed, 117 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
new file mode 100644
index 000..271554a
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/allwinner,sun7i-gmac.txt
@@ -0,0 +1,22 @@
+* Allwinner GMAC ethernet controller
+
+This device is a platform glue layer for stmmac.
+Please see stmmac.txt for the other unchanged properties.
+
+Required properties:
+ - compatible: Should be allwinner,sun7i-gmac
Please use sun7i-a20-gmac here.
+ - reg: Address and length of register set for the device and corresponding
+ clock control
+Examples:
+
+ gmac: ethernet@01c5 {
+ compatible = allwinner,sun7i-gmac;
+ reg = 0x01c5 0x1,
+ 0x01c20164 0x4;
This is actually a clock, and should probably be registered in the
common clock framework.
Mike: This small register actually is a regular muxer/divider, except
that it has some bits that are of interest to the ethernet controller
(for example to set wether it's using GMII or RGMII to communicate
with the phy), that, as far as I'm aware of, aren't really fitting
into the CCF.
Do you have some recommendation on how to proceed?
Maybe make a thin real clock driver in this hardware glue, that
provides !exported function to set this *GMII thing.
Is this register part of a bigger IP block that manages clocks for other
IP blocks than stmmac as well? If not, I don't see a point of exporting
a clock from inside of the GMAC domain just to feed it back into it
as the only user.
This register is actually part of the SoC clock controller. So it sits
right beside the other clocks registers controlling the clocks of the
other devices, and is not part of the GMAC IP itself.
Is there any description for GMAC_IF_TYPE_RGMII and GMAC_TX_CLK fields?
Name of the latter sounds like a normal clock mux, but the former is
just a mystery (especially why it is a part of the clock controller).
You can find the register documented on page 89, table 1.5.4.65 of
http://dl.linux-sunxi.org/A20/A20%20User%20Manual%202013-03-22.pdf
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
