| Message ID | 20180411141641.14675-4-icenowy@aosc.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: > From: Chen-Yu Tsai <wens@csie.org> > > On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU > address space; on the A64 SoC this register is in the SRAM controller > address space, and with a different offset. > > To access the register from another device and hide the internal > difference between the device, let it register a regmap named > "emac-clock". We can then get the device from the phandle, and > retrieve the regmap with dev_get_regmap(); in this situation the > regmap_field will be set up to access the only register in the regmap. > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > [Icenowy: change to use regmaps with single register, change commit > message] > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 ++++++++++++++++++++++- > 1 file changed, 46 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > index 1037f6c78bca..b61210c0d415 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { > .msb = 31, > }; > > +/* Specially exported regmap which contains only EMAC register */ > +const struct reg_field single_reg_field = { > + .reg = 0, > + .lsb = 0, > + .msb = 31, > +}; > + I'm not sure this would be wise. If we ever need some other register exported through the regmap, will have to change all the calling sites everywhere in the kernel, which will be a pain and will break bisectability. Chen-Yu's (or was it yours?) initial solution with a custom writeable hook only allowing a single register seemed like a better one. Maxime
于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >> From: Chen-Yu Tsai <wens@csie.org> >> >> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU >> address space; on the A64 SoC this register is in the SRAM controller >> address space, and with a different offset. >> >> To access the register from another device and hide the internal >> difference between the device, let it register a regmap named >> "emac-clock". We can then get the device from the phandle, and >> retrieve the regmap with dev_get_regmap(); in this situation the >> regmap_field will be set up to access the only register in the >regmap. >> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> [Icenowy: change to use regmaps with single register, change commit >> message] >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> --- >> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >++++++++++++++++++++++- >> 1 file changed, 46 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> index 1037f6c78bca..b61210c0d415 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { >> .msb = 31, >> }; >> >> +/* Specially exported regmap which contains only EMAC register */ >> +const struct reg_field single_reg_field = { >> + .reg = 0, >> + .lsb = 0, >> + .msb = 31, >> +}; >> + > >I'm not sure this would be wise. If we ever need some other register >exported through the regmap, will have to change all the calling sites >everywhere in the kernel, which will be a pain and will break >bisectability. In this situation the register can be exported as another regmap. Currently the code will access a regmap with name "emac-clock" for this register. > >Chen-Yu's (or was it yours?) initial solution with a custom writeable >hook only allowing a single register seemed like a better one. But I remember you mentioned that you want it to hide the difference inside the device. > >Maxime
On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >>> From: Chen-Yu Tsai <wens@csie.org> >>> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU >>> address space; on the A64 SoC this register is in the SRAM controller >>> address space, and with a different offset. >>> >>> To access the register from another device and hide the internal >>> difference between the device, let it register a regmap named >>> "emac-clock". We can then get the device from the phandle, and >>> retrieve the regmap with dev_get_regmap(); in this situation the >>> regmap_field will be set up to access the only register in the >>regmap. >>> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> [Icenowy: change to use regmaps with single register, change commit >>> message] >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >>> --- >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >>++++++++++++++++++++++- >>> 1 file changed, 46 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> index 1037f6c78bca..b61210c0d415 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { >>> .msb = 31, >>> }; >>> >>> +/* Specially exported regmap which contains only EMAC register */ >>> +const struct reg_field single_reg_field = { >>> + .reg = 0, >>> + .lsb = 0, >>> + .msb = 31, >>> +}; >>> + >> >>I'm not sure this would be wise. If we ever need some other register >>exported through the regmap, will have to change all the calling sites >>everywhere in the kernel, which will be a pain and will break >>bisectability. > > In this situation the register can be exported as another > regmap. Currently the code will access a regmap with name > "emac-clock" for this register. > >> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable >>hook only allowing a single register seemed like a better one. > > But I remember you mentioned that you want it to hide the > difference inside the device. Hi, The idea is that a device can export multiple regmaps. This one, the one named "gmac" (in my soon to come v2) or "emac-clock" here, is but one of many possible regmaps, and it only exports the register needed by the GMAC/EMAC. IMHO it is highly unlikely the same piece of hardware would need a second register from the same device. A more likely situation would be it needs another register from a different device, like on the H6 where the "internal PHY" is not so internal from a system bus point of view. ChenYu
Hi Chen-Yu,
I love your patch! Perhaps something to improve:
[auto build test WARNING on robh/for-next]
[also build test WARNING on v4.16 next-20180412]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Icenowy-Zheng/Add-support-in-dwmac-sun8i-for-accessing-EMAC-clock/20180413-004816
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c:82:24: sparse: symbol 'old_syscon_reg_field' was not declared. Should it be static?
>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c:89:24: sparse: symbol 'single_reg_field' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: > On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: > >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: > >>> From: Chen-Yu Tsai <wens@csie.org> > >>> > >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU > >>> address space; on the A64 SoC this register is in the SRAM controller > >>> address space, and with a different offset. > >>> > >>> To access the register from another device and hide the internal > >>> difference between the device, let it register a regmap named > >>> "emac-clock". We can then get the device from the phandle, and > >>> retrieve the regmap with dev_get_regmap(); in this situation the > >>> regmap_field will be set up to access the only register in the > >>regmap. > >>> > >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >>> [Icenowy: change to use regmaps with single register, change commit > >>> message] > >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >>> --- > >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 > >>++++++++++++++++++++++- > >>> 1 file changed, 46 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >>> index 1037f6c78bca..b61210c0d415 100644 > >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { > >>> .msb = 31, > >>> }; > >>> > >>> +/* Specially exported regmap which contains only EMAC register */ > >>> +const struct reg_field single_reg_field = { > >>> + .reg = 0, > >>> + .lsb = 0, > >>> + .msb = 31, > >>> +}; > >>> + > >> > >>I'm not sure this would be wise. If we ever need some other register > >>exported through the regmap, will have to change all the calling sites > >>everywhere in the kernel, which will be a pain and will break > >>bisectability. > > > > In this situation the register can be exported as another > > regmap. Currently the code will access a regmap with name > > "emac-clock" for this register. > > > >> > >>Chen-Yu's (or was it yours?) initial solution with a custom writeable > >>hook only allowing a single register seemed like a better one. > > > > But I remember you mentioned that you want it to hide the > > difference inside the device. > > The idea is that a device can export multiple regmaps. This one, > the one named "gmac" (in my soon to come v2) or "emac-clock" here, > is but one of many possible regmaps, and it only exports the register > needed by the GMAC/EMAC. I'm not sure this would be wise either. There's a single register map, and as far as I know we don't have a binding to express this in the DT. This means that the customer and provider would have to use the same name, but without anything actually enforcing it aside from "someone in the community knows it". This is not a really good design, and I was actually preferring your first option. We shouldn't rely on any undocumented rule. This will be easy to break and hard to maintain. Maxime
于 2018年4月16日 GMT+08:00 下午10:31:30, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> >wrote: >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard ><maxime.ripard@bootlin.com> 写到: >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >> >>> From: Chen-Yu Tsai <wens@csie.org> >> >>> >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU >> >>> address space; on the A64 SoC this register is in the SRAM >controller >> >>> address space, and with a different offset. >> >>> >> >>> To access the register from another device and hide the internal >> >>> difference between the device, let it register a regmap named >> >>> "emac-clock". We can then get the device from the phandle, and >> >>> retrieve the regmap with dev_get_regmap(); in this situation the >> >>> regmap_field will be set up to access the only register in the >> >>regmap. >> >>> >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> >>> [Icenowy: change to use regmaps with single register, change >commit >> >>> message] >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> >>> --- >> >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >> >>++++++++++++++++++++++- >> >>> 1 file changed, 46 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> index 1037f6c78bca..b61210c0d415 100644 >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = >{ >> >>> .msb = 31, >> >>> }; >> >>> >> >>> +/* Specially exported regmap which contains only EMAC register >*/ >> >>> +const struct reg_field single_reg_field = { >> >>> + .reg = 0, >> >>> + .lsb = 0, >> >>> + .msb = 31, >> >>> +}; >> >>> + >> >> >> >>I'm not sure this would be wise. If we ever need some other >register >> >>exported through the regmap, will have to change all the calling >sites >> >>everywhere in the kernel, which will be a pain and will break >> >>bisectability. >> > >> > In this situation the register can be exported as another >> > regmap. Currently the code will access a regmap with name >> > "emac-clock" for this register. >> > >> >> >> >>Chen-Yu's (or was it yours?) initial solution with a custom >writeable >> >>hook only allowing a single register seemed like a better one. >> > >> > But I remember you mentioned that you want it to hide the >> > difference inside the device. >> >> The idea is that a device can export multiple regmaps. This one, >> the one named "gmac" (in my soon to come v2) or "emac-clock" here, >> is but one of many possible regmaps, and it only exports the register >> needed by the GMAC/EMAC. > >I'm not sure this would be wise either. There's a single register map, >and as far as I know we don't have a binding to express this in the >DT. This means that the customer and provider would have to use the >same name, but without anything actually enforcing it aside from >"someone in the community knows it". > >This is not a really good design, and I was actually preferring your >first option. We shouldn't rely on any undocumented rule. This will be >easy to break and hard to maintain. Okay. Then I will revert back to the original solution in the next version. > >Maxime
On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> wrote: >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >> >>> From: Chen-Yu Tsai <wens@csie.org> >> >>> >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU >> >>> address space; on the A64 SoC this register is in the SRAM controller >> >>> address space, and with a different offset. >> >>> >> >>> To access the register from another device and hide the internal >> >>> difference between the device, let it register a regmap named >> >>> "emac-clock". We can then get the device from the phandle, and >> >>> retrieve the regmap with dev_get_regmap(); in this situation the >> >>> regmap_field will be set up to access the only register in the >> >>regmap. >> >>> >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> >>> [Icenowy: change to use regmaps with single register, change commit >> >>> message] >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> >>> --- >> >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >> >>++++++++++++++++++++++- >> >>> 1 file changed, 46 insertions(+), 2 deletions(-) >> >>> >> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> index 1037f6c78bca..b61210c0d415 100644 >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { >> >>> .msb = 31, >> >>> }; >> >>> >> >>> +/* Specially exported regmap which contains only EMAC register */ >> >>> +const struct reg_field single_reg_field = { >> >>> + .reg = 0, >> >>> + .lsb = 0, >> >>> + .msb = 31, >> >>> +}; >> >>> + >> >> >> >>I'm not sure this would be wise. If we ever need some other register >> >>exported through the regmap, will have to change all the calling sites >> >>everywhere in the kernel, which will be a pain and will break >> >>bisectability. >> > >> > In this situation the register can be exported as another >> > regmap. Currently the code will access a regmap with name >> > "emac-clock" for this register. >> > >> >> >> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable >> >>hook only allowing a single register seemed like a better one. >> > >> > But I remember you mentioned that you want it to hide the >> > difference inside the device. >> >> The idea is that a device can export multiple regmaps. This one, >> the one named "gmac" (in my soon to come v2) or "emac-clock" here, >> is but one of many possible regmaps, and it only exports the register >> needed by the GMAC/EMAC. > > I'm not sure this would be wise either. There's a single register map, > and as far as I know we don't have a binding to express this in the > DT. This means that the customer and provider would have to use the > same name, but without anything actually enforcing it aside from > "someone in the community knows it". > > This is not a really good design, and I was actually preferring your > first option. We shouldn't rely on any undocumented rule. This will be > easy to break and hard to maintain. So, one regmap per device covering the whole register range, and the consumer knows which register to poke by looking at its own compatible. That sound right? ChenYu
On Mon, Apr 16, 2018 at 10:51:55PM +0800, Chen-Yu Tsai wrote: > On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard > <maxime.ripard@bootlin.com> wrote: > > On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: > >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: > >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: > >> >>> From: Chen-Yu Tsai <wens@csie.org> > >> >>> > >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU > >> >>> address space; on the A64 SoC this register is in the SRAM controller > >> >>> address space, and with a different offset. > >> >>> > >> >>> To access the register from another device and hide the internal > >> >>> difference between the device, let it register a regmap named > >> >>> "emac-clock". We can then get the device from the phandle, and > >> >>> retrieve the regmap with dev_get_regmap(); in this situation the > >> >>> regmap_field will be set up to access the only register in the > >> >>regmap. > >> >>> > >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >> >>> [Icenowy: change to use regmaps with single register, change commit > >> >>> message] > >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >> >>> --- > >> >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 > >> >>++++++++++++++++++++++- > >> >>> 1 file changed, 46 insertions(+), 2 deletions(-) > >> >>> > >> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >> >>> index 1037f6c78bca..b61210c0d415 100644 > >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c > >> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { > >> >>> .msb = 31, > >> >>> }; > >> >>> > >> >>> +/* Specially exported regmap which contains only EMAC register */ > >> >>> +const struct reg_field single_reg_field = { > >> >>> + .reg = 0, > >> >>> + .lsb = 0, > >> >>> + .msb = 31, > >> >>> +}; > >> >>> + > >> >> > >> >>I'm not sure this would be wise. If we ever need some other register > >> >>exported through the regmap, will have to change all the calling sites > >> >>everywhere in the kernel, which will be a pain and will break > >> >>bisectability. > >> > > >> > In this situation the register can be exported as another > >> > regmap. Currently the code will access a regmap with name > >> > "emac-clock" for this register. > >> > > >> >> > >> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable > >> >>hook only allowing a single register seemed like a better one. > >> > > >> > But I remember you mentioned that you want it to hide the > >> > difference inside the device. > >> > >> The idea is that a device can export multiple regmaps. This one, > >> the one named "gmac" (in my soon to come v2) or "emac-clock" here, > >> is but one of many possible regmaps, and it only exports the register > >> needed by the GMAC/EMAC. > > > > I'm not sure this would be wise either. There's a single register map, > > and as far as I know we don't have a binding to express this in the > > DT. This means that the customer and provider would have to use the > > same name, but without anything actually enforcing it aside from > > "someone in the community knows it". > > > > This is not a really good design, and I was actually preferring your > > first option. We shouldn't rely on any undocumented rule. This will be > > easy to break and hard to maintain. > > So, one regmap per device covering the whole register range, and the > consumer knows which register to poke by looking at its own compatible. > > That sound right? Yep. And ideally, sending a single serie for both the A64 and the R40 cases, in order to provide the big picture. Maxime
On Tue, Apr 17, 2018 at 7:52 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Mon, Apr 16, 2018 at 10:51:55PM +0800, Chen-Yu Tsai wrote: >> On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard >> <maxime.ripard@bootlin.com> wrote: >> > On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: >> >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> wrote: >> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >> >> >>> From: Chen-Yu Tsai <wens@csie.org> >> >> >>> >> >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU >> >> >>> address space; on the A64 SoC this register is in the SRAM controller >> >> >>> address space, and with a different offset. >> >> >>> >> >> >>> To access the register from another device and hide the internal >> >> >>> difference between the device, let it register a regmap named >> >> >>> "emac-clock". We can then get the device from the phandle, and >> >> >>> retrieve the regmap with dev_get_regmap(); in this situation the >> >> >>> regmap_field will be set up to access the only register in the >> >> >>regmap. >> >> >>> >> >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> >> >>> [Icenowy: change to use regmaps with single register, change commit >> >> >>> message] >> >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> >> >>> --- >> >> >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >> >> >>++++++++++++++++++++++- >> >> >>> 1 file changed, 46 insertions(+), 2 deletions(-) >> >> >>> >> >> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >> >>> index 1037f6c78bca..b61210c0d415 100644 >> >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >> >> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { >> >> >>> .msb = 31, >> >> >>> }; >> >> >>> >> >> >>> +/* Specially exported regmap which contains only EMAC register */ >> >> >>> +const struct reg_field single_reg_field = { >> >> >>> + .reg = 0, >> >> >>> + .lsb = 0, >> >> >>> + .msb = 31, >> >> >>> +}; >> >> >>> + >> >> >> >> >> >>I'm not sure this would be wise. If we ever need some other register >> >> >>exported through the regmap, will have to change all the calling sites >> >> >>everywhere in the kernel, which will be a pain and will break >> >> >>bisectability. >> >> > >> >> > In this situation the register can be exported as another >> >> > regmap. Currently the code will access a regmap with name >> >> > "emac-clock" for this register. >> >> > >> >> >> >> >> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable >> >> >>hook only allowing a single register seemed like a better one. >> >> > >> >> > But I remember you mentioned that you want it to hide the >> >> > difference inside the device. >> >> >> >> The idea is that a device can export multiple regmaps. This one, >> >> the one named "gmac" (in my soon to come v2) or "emac-clock" here, >> >> is but one of many possible regmaps, and it only exports the register >> >> needed by the GMAC/EMAC. >> > >> > I'm not sure this would be wise either. There's a single register map, >> > and as far as I know we don't have a binding to express this in the >> > DT. This means that the customer and provider would have to use the >> > same name, but without anything actually enforcing it aside from >> > "someone in the community knows it". >> > >> > This is not a really good design, and I was actually preferring your >> > first option. We shouldn't rely on any undocumented rule. This will be >> > easy to break and hard to maintain. >> >> So, one regmap per device covering the whole register range, and the >> consumer knows which register to poke by looking at its own compatible. >> >> That sound right? > > Yep. And ideally, sending a single serie for both the A64 and the R40 > cases, in order to provide the big picture. OK. I'll incorporate Icenowy's stuff into my series. ChenYu
于 2018年4月17日 GMT+08:00 下午7:59:38, Chen-Yu Tsai <wens@csie.org> 写到: >On Tue, Apr 17, 2018 at 7:52 PM, Maxime Ripard ><maxime.ripard@bootlin.com> wrote: >> On Mon, Apr 16, 2018 at 10:51:55PM +0800, Chen-Yu Tsai wrote: >>> On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard >>> <maxime.ripard@bootlin.com> wrote: >>> > On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote: >>> >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icenowy@aosc.io> >wrote: >>> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard ><maxime.ripard@bootlin.com> 写到: >>> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote: >>> >> >>> From: Chen-Yu Tsai <wens@csie.org> >>> >> >>> >>> >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the >CCU >>> >> >>> address space; on the A64 SoC this register is in the SRAM >controller >>> >> >>> address space, and with a different offset. >>> >> >>> >>> >> >>> To access the register from another device and hide the >internal >>> >> >>> difference between the device, let it register a regmap named >>> >> >>> "emac-clock". We can then get the device from the phandle, >and >>> >> >>> retrieve the regmap with dev_get_regmap(); in this situation >the >>> >> >>> regmap_field will be set up to access the only register in >the >>> >> >>regmap. >>> >> >>> >>> >> >>> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> >> >>> [Icenowy: change to use regmaps with single register, change >commit >>> >> >>> message] >>> >> >>> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >>> >> >>> --- >>> >> >>> drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 >>> >> >>++++++++++++++++++++++- >>> >> >>> 1 file changed, 46 insertions(+), 2 deletions(-) >>> >> >>> >>> >> >>> diff --git >a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> >> >>> index 1037f6c78bca..b61210c0d415 100644 >>> >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c >>> >> >>> @@ -85,6 +85,13 @@ const struct reg_field >old_syscon_reg_field = { >>> >> >>> .msb = 31, >>> >> >>> }; >>> >> >>> >>> >> >>> +/* Specially exported regmap which contains only EMAC >register */ >>> >> >>> +const struct reg_field single_reg_field = { >>> >> >>> + .reg = 0, >>> >> >>> + .lsb = 0, >>> >> >>> + .msb = 31, >>> >> >>> +}; >>> >> >>> + >>> >> >> >>> >> >>I'm not sure this would be wise. If we ever need some other >register >>> >> >>exported through the regmap, will have to change all the >calling sites >>> >> >>everywhere in the kernel, which will be a pain and will break >>> >> >>bisectability. >>> >> > >>> >> > In this situation the register can be exported as another >>> >> > regmap. Currently the code will access a regmap with name >>> >> > "emac-clock" for this register. >>> >> > >>> >> >> >>> >> >>Chen-Yu's (or was it yours?) initial solution with a custom >writeable >>> >> >>hook only allowing a single register seemed like a better one. >>> >> > >>> >> > But I remember you mentioned that you want it to hide the >>> >> > difference inside the device. >>> >> >>> >> The idea is that a device can export multiple regmaps. This one, >>> >> the one named "gmac" (in my soon to come v2) or "emac-clock" >here, >>> >> is but one of many possible regmaps, and it only exports the >register >>> >> needed by the GMAC/EMAC. >>> > >>> > I'm not sure this would be wise either. There's a single register >map, >>> > and as far as I know we don't have a binding to express this in >the >>> > DT. This means that the customer and provider would have to use >the >>> > same name, but without anything actually enforcing it aside from >>> > "someone in the community knows it". >>> > >>> > This is not a really good design, and I was actually preferring >your >>> > first option. We shouldn't rely on any undocumented rule. This >will be >>> > easy to break and hard to maintain. >>> >>> So, one regmap per device covering the whole register range, and the >>> consumer knows which register to poke by looking at its own >compatible. >>> >>> That sound right? >> >> Yep. And ideally, sending a single serie for both the A64 and the R40 >> cases, in order to provide the big picture. > >OK. I'll incorporate Icenowy's stuff into my series. In this situation maybe I should send newer revision of A64 drivers to you? > >ChenYu > >_______________________________________________ >linux-arm-kernel mailing list >linux-arm-kernel@lists.infradead.org >http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c index 1037f6c78bca..b61210c0d415 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = { .msb = 31, }; +/* Specially exported regmap which contains only EMAC register */ +const struct reg_field single_reg_field = { + .reg = 0, + .lsb = 0, + .msb = 31, +}; + static const struct emac_variant emac_variant_h3 = { .default_syscon_value = 0x58000, .soc_has_internal_phy = true, @@ -979,6 +986,44 @@ static struct mac_device_info *sun8i_dwmac_setup(void *ppriv) return mac; } +static struct regmap *sun8i_dwmac_get_emac_regmap(struct sunxi_priv_data *emac, + struct device_node *node) +{ + struct device_node *syscon_node; + struct platform_device *syscon_pdev = NULL; + struct regmap *regmap = NULL; + + syscon_node = of_parse_phandle(node, "syscon", 0); + if (!syscon_node) + return ERR_PTR(-ENODEV); + + if (of_device_is_compatible(syscon_node, "syscon")) { + regmap = syscon_regmap_lookup_by_phandle(node, "syscon"); + + emac->emac_reg_field = &old_syscon_reg_field; + } else { + syscon_pdev = of_find_device_by_node(syscon_node); + if (!syscon_pdev) { + /* platform device might not be probed yet */ + regmap = ERR_PTR(-EPROBE_DEFER); + goto out_put_node; + } + + /* If no regmap is found then trap into error */ + regmap = dev_get_regmap(&syscon_pdev->dev, "emac-clock"); + if (!regmap) + regmap = ERR_PTR(-EINVAL); + + emac->emac_reg_field = &single_reg_field; + } + + if (syscon_pdev) + platform_device_put(syscon_pdev); +out_put_node: + of_node_put(syscon_node); + return regmap; +} + static int sun8i_dwmac_probe(struct platform_device *pdev) { struct plat_stmmacenet_data *plat_dat; @@ -1023,13 +1068,12 @@ static int sun8i_dwmac_probe(struct platform_device *pdev) gmac->regulator = NULL; } - regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "syscon"); + regmap = sun8i_dwmac_get_emac_regmap(gmac, pdev->dev.of_node); if (IS_ERR(regmap)) { ret = PTR_ERR(regmap); dev_err(&pdev->dev, "Unable to map syscon: %d\n", ret); return ret; } - gmac->emac_reg_field = old_syscon_reg_field; gmac->regmap_field = devm_regmap_field_alloc(dev, regmap, *gmac->emac_reg_field);