| Message ID | 20180317092857.4396-3-wens@csie.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | ARM: sun8i: r40: Add Ethernet support | expand |
On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: > From: Icenowy Zheng <icenowy@aosc.io> > > There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in > the syscon part, in the CCU of R40 SoC. > > Export a regmap of the CCU. > > Read access is not restricted to all registers, but only the GMAC > register is allowed to be written. > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > Signed-off-by: Chen-Yu Tsai <wens@csie.org> Gah, this is crazy. I'm really starting to regret letting that syscon in in the first place... And I'm not really looking forward the time where SCPI et al. will be mature and we'll have the clock controller completely outside of our control. Maxime
On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >> From: Icenowy Zheng <icenowy@aosc.io> >> >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in >> the syscon part, in the CCU of R40 SoC. >> >> Export a regmap of the CCU. >> >> Read access is not restricted to all registers, but only the GMAC >> register is allowed to be written. >> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > Gah, this is crazy. I'm really starting to regret letting that syscon > in in the first place... IMHO syscon is really a better fit. It's part of the glue layer and most other dwmac user platforms treat it as such and use a syscon. Plus the controls encompass delays (phase), inverters (polarity), and even signal routing. It's not really just a group of clock controls, like what we poorly modeled for A20/A31. I think that was really a mistake. As I mentioned in the cover letter, a slightly saner approach would be to let drivers add custom syscon entries, which would then require less custom plumbing. > And I'm not really looking forward the time where SCPI et al. will be > mature and we'll have the clock controller completely outside of our > control. I don't think it's going to happen for any of the older SoCs. The R40 only stands out because the GMAC controls are in the clock controller address space, presumably to be like the A20. ChenYu
On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard > <maxime.ripard@bootlin.com> wrote: > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: > >> From: Icenowy Zheng <icenowy@aosc.io> > >> > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in > >> the syscon part, in the CCU of R40 SoC. > >> > >> Export a regmap of the CCU. > >> > >> Read access is not restricted to all registers, but only the GMAC > >> register is allowed to be written. > >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > > > Gah, this is crazy. I'm really starting to regret letting that syscon > > in in the first place... > > IMHO syscon is really a better fit. It's part of the glue layer and > most other dwmac user platforms treat it as such and use a syscon. > Plus the controls encompass delays (phase), inverters (polarity), > and even signal routing. It's not really just a group of clock controls, > like what we poorly modeled for A20/A31. I think that was really a > mistake. > > As I mentioned in the cover letter, a slightly saner approach would > be to let drivers add custom syscon entries, which would then require > less custom plumbing. A syscon is convenient, sure, but it also bypasses any abstraction layer we have everywhere else, which means that we'll have to maintain the register layout in each and every driver that uses it. So far, it's only be the GMAC, but it can also be others (the SRAM controller comes to my mind), and then, if there's any difference in the design in a future SoC, we'll have to maintain that in the GMAC driver as well. > > And I'm not really looking forward the time where SCPI et al. will be > > mature and we'll have the clock controller completely outside of our > > control. > > I don't think it's going to happen for any of the older SoCs. The R40 > only stands out because the GMAC controls are in the clock controller > address space, presumably to be like the A20. SCPI (or equivalent) is a really nice feature to have when it comes to virtualization, so even if it's less likely, it doesn't make it less relevant on other SoCs. Maxime
On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: > On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: > > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard > > <maxime.ripard@bootlin.com> wrote: > > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: > > >> From: Icenowy Zheng <icenowy@aosc.io> > > >> > > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in > > >> the syscon part, in the CCU of R40 SoC. > > >> > > >> Export a regmap of the CCU. > > >> > > >> Read access is not restricted to all registers, but only the GMAC > > >> register is allowed to be written. > > >> > > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > > > > > Gah, this is crazy. I'm really starting to regret letting that syscon > > > in in the first place... > > > > IMHO syscon is really a better fit. It's part of the glue layer and > > most other dwmac user platforms treat it as such and use a syscon. > > Plus the controls encompass delays (phase), inverters (polarity), > > and even signal routing. It's not really just a group of clock controls, > > like what we poorly modeled for A20/A31. I think that was really a > > mistake. > > > > As I mentioned in the cover letter, a slightly saner approach would > > be to let drivers add custom syscon entries, which would then require > > less custom plumbing. > > A syscon is convenient, sure, but it also bypasses any abstraction > layer we have everywhere else, which means that we'll have to maintain > the register layout in each and every driver that uses it. > > So far, it's only be the GMAC, but it can also be others (the SRAM > controller comes to my mind), and then, if there's any difference in > the design in a future SoC, we'll have to maintain that in the GMAC > driver as well. I guess I forgot to say something, I'm fine with using a syscon we already have. I'm just questionning if merging any other driver using one is the right move. Maxime
于 2018年4月3日 GMT+08:00 下午5:50:05, Maxime Ripard <maxime.ripard@bootlin.com> 写到: >On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: >> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: >> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard >> > <maxime.ripard@bootlin.com> wrote: >> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >> > >> From: Icenowy Zheng <icenowy@aosc.io> >> > >> >> > >> There's a GMAC configuration register, which exists on >A64/A83T/H3/H5 in >> > >> the syscon part, in the CCU of R40 SoC. >> > >> >> > >> Export a regmap of the CCU. >> > >> >> > >> Read access is not restricted to all registers, but only the >GMAC >> > >> register is allowed to be written. >> > >> >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> > > >> > > Gah, this is crazy. I'm really starting to regret letting that >syscon >> > > in in the first place... >> > >> > IMHO syscon is really a better fit. It's part of the glue layer and >> > most other dwmac user platforms treat it as such and use a syscon. >> > Plus the controls encompass delays (phase), inverters (polarity), >> > and even signal routing. It's not really just a group of clock >controls, >> > like what we poorly modeled for A20/A31. I think that was really a >> > mistake. >> > >> > As I mentioned in the cover letter, a slightly saner approach would >> > be to let drivers add custom syscon entries, which would then >require >> > less custom plumbing. >> >> A syscon is convenient, sure, but it also bypasses any abstraction >> layer we have everywhere else, which means that we'll have to >maintain >> the register layout in each and every driver that uses it. >> >> So far, it's only be the GMAC, but it can also be others (the SRAM >> controller comes to my mind), and then, if there's any difference in >> the design in a future SoC, we'll have to maintain that in the GMAC >> driver as well. > >I guess I forgot to say something, I'm fine with using a syscon we >already have. > >I'm just questionning if merging any other driver using one is the >right move. Even for current SoCs supported by dwnac-sun8i, there is a syscon/sram-controller problem. They're both at 0x1c00000. The first examples for the need of sram-controller is A64, which we need to claim SRAM C for DE2 access. > >Maxime
On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: >> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: >> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard >> > <maxime.ripard@bootlin.com> wrote: >> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >> > >> From: Icenowy Zheng <icenowy@aosc.io> >> > >> >> > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in >> > >> the syscon part, in the CCU of R40 SoC. >> > >> >> > >> Export a regmap of the CCU. >> > >> >> > >> Read access is not restricted to all registers, but only the GMAC >> > >> register is allowed to be written. >> > >> >> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> > > >> > > Gah, this is crazy. I'm really starting to regret letting that syscon >> > > in in the first place... >> > >> > IMHO syscon is really a better fit. It's part of the glue layer and >> > most other dwmac user platforms treat it as such and use a syscon. >> > Plus the controls encompass delays (phase), inverters (polarity), >> > and even signal routing. It's not really just a group of clock controls, >> > like what we poorly modeled for A20/A31. I think that was really a >> > mistake. >> > >> > As I mentioned in the cover letter, a slightly saner approach would >> > be to let drivers add custom syscon entries, which would then require >> > less custom plumbing. >> >> A syscon is convenient, sure, but it also bypasses any abstraction >> layer we have everywhere else, which means that we'll have to maintain >> the register layout in each and every driver that uses it. >> >> So far, it's only be the GMAC, but it can also be others (the SRAM >> controller comes to my mind), and then, if there's any difference in >> the design in a future SoC, we'll have to maintain that in the GMAC >> driver as well. > > I guess I forgot to say something, I'm fine with using a syscon we > already have. > > I'm just questionning if merging any other driver using one is the > right move. Right. So in this case, we are not actually going through the syscon API. Rather we are exporting a regmap whose properties we actually define. If it makes you more acceptable to it, we could map just the GMAC register in the new regmap, and also have it named. This is all plumbing within the kernel so the device tree stays the same. ChenYu
于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到: >On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard ><maxime.ripard@bootlin.com> wrote: >> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: >>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: >>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard >>> > <maxime.ripard@bootlin.com> wrote: >>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >>> > >> From: Icenowy Zheng <icenowy@aosc.io> >>> > >> >>> > >> There's a GMAC configuration register, which exists on >A64/A83T/H3/H5 in >>> > >> the syscon part, in the CCU of R40 SoC. >>> > >> >>> > >> Export a regmap of the CCU. >>> > >> >>> > >> Read access is not restricted to all registers, but only the >GMAC >>> > >> register is allowed to be written. >>> > >> >>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>> > > >>> > > Gah, this is crazy. I'm really starting to regret letting that >syscon >>> > > in in the first place... >>> > >>> > IMHO syscon is really a better fit. It's part of the glue layer >and >>> > most other dwmac user platforms treat it as such and use a syscon. >>> > Plus the controls encompass delays (phase), inverters (polarity), >>> > and even signal routing. It's not really just a group of clock >controls, >>> > like what we poorly modeled for A20/A31. I think that was really a >>> > mistake. >>> > >>> > As I mentioned in the cover letter, a slightly saner approach >would >>> > be to let drivers add custom syscon entries, which would then >require >>> > less custom plumbing. >>> >>> A syscon is convenient, sure, but it also bypasses any abstraction >>> layer we have everywhere else, which means that we'll have to >maintain >>> the register layout in each and every driver that uses it. >>> >>> So far, it's only be the GMAC, but it can also be others (the SRAM >>> controller comes to my mind), and then, if there's any difference in >>> the design in a future SoC, we'll have to maintain that in the GMAC >>> driver as well. >> >> I guess I forgot to say something, I'm fine with using a syscon we >> already have. >> >> I'm just questionning if merging any other driver using one is the >> right move. > >Right. So in this case, we are not actually going through the syscon >API. Rather we are exporting a regmap whose properties we actually >define. If it makes you more acceptable to it, we could map just >the GMAC register in the new regmap, and also have it named. This >is all plumbing within the kernel so the device tree stays the same. I think my driver has already restricted the write permission only to GMAC register. > >ChenYu
On Tue, Apr 3, 2018 at 5:54 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > 于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到: >>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard >><maxime.ripard@bootlin.com> wrote: >>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: >>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: >>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard >>>> > <maxime.ripard@bootlin.com> wrote: >>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >>>> > >> From: Icenowy Zheng <icenowy@aosc.io> >>>> > >> >>>> > >> There's a GMAC configuration register, which exists on >>A64/A83T/H3/H5 in >>>> > >> the syscon part, in the CCU of R40 SoC. >>>> > >> >>>> > >> Export a regmap of the CCU. >>>> > >> >>>> > >> Read access is not restricted to all registers, but only the >>GMAC >>>> > >> register is allowed to be written. >>>> > >> >>>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >>>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> >>>> > > >>>> > > Gah, this is crazy. I'm really starting to regret letting that >>syscon >>>> > > in in the first place... >>>> > >>>> > IMHO syscon is really a better fit. It's part of the glue layer >>and >>>> > most other dwmac user platforms treat it as such and use a syscon. >>>> > Plus the controls encompass delays (phase), inverters (polarity), >>>> > and even signal routing. It's not really just a group of clock >>controls, >>>> > like what we poorly modeled for A20/A31. I think that was really a >>>> > mistake. >>>> > >>>> > As I mentioned in the cover letter, a slightly saner approach >>would >>>> > be to let drivers add custom syscon entries, which would then >>require >>>> > less custom plumbing. >>>> >>>> A syscon is convenient, sure, but it also bypasses any abstraction >>>> layer we have everywhere else, which means that we'll have to >>maintain >>>> the register layout in each and every driver that uses it. >>>> >>>> So far, it's only be the GMAC, but it can also be others (the SRAM >>>> controller comes to my mind), and then, if there's any difference in >>>> the design in a future SoC, we'll have to maintain that in the GMAC >>>> driver as well. >>> >>> I guess I forgot to say something, I'm fine with using a syscon we >>> already have. >>> >>> I'm just questionning if merging any other driver using one is the >>> right move. >> >>Right. So in this case, we are not actually going through the syscon >>API. Rather we are exporting a regmap whose properties we actually >>define. If it makes you more acceptable to it, we could map just >>the GMAC register in the new regmap, and also have it named. This >>is all plumbing within the kernel so the device tree stays the same. > > I think my driver has already restricted the write permission > only to GMAC register. Correct, but it still maps the entire region out, which means the consumer needs to know which offset to use. Maxime is saying this is something that is troublesome to maintain. So my proposal was to create a regmap with a base at the GMAC register offset. That way, the consumer doesn't need to use an offset to access it. ChenYu
On Tue, Apr 03, 2018 at 05:58:05PM +0800, Chen-Yu Tsai wrote: > On Tue, Apr 3, 2018 at 5:54 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > > > > > > 于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <wens@csie.org> 写到: > >>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard > >><maxime.ripard@bootlin.com> wrote: > >>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: > >>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: > >>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard > >>>> > <maxime.ripard@bootlin.com> wrote: > >>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: > >>>> > >> From: Icenowy Zheng <icenowy@aosc.io> > >>>> > >> > >>>> > >> There's a GMAC configuration register, which exists on > >>A64/A83T/H3/H5 in > >>>> > >> the syscon part, in the CCU of R40 SoC. > >>>> > >> > >>>> > >> Export a regmap of the CCU. > >>>> > >> > >>>> > >> Read access is not restricted to all registers, but only the > >>GMAC > >>>> > >> register is allowed to be written. > >>>> > >> > >>>> > >> Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > >>>> > >> Signed-off-by: Chen-Yu Tsai <wens@csie.org> > >>>> > > > >>>> > > Gah, this is crazy. I'm really starting to regret letting that > >>syscon > >>>> > > in in the first place... > >>>> > > >>>> > IMHO syscon is really a better fit. It's part of the glue layer > >>and > >>>> > most other dwmac user platforms treat it as such and use a syscon. > >>>> > Plus the controls encompass delays (phase), inverters (polarity), > >>>> > and even signal routing. It's not really just a group of clock > >>controls, > >>>> > like what we poorly modeled for A20/A31. I think that was really a > >>>> > mistake. > >>>> > > >>>> > As I mentioned in the cover letter, a slightly saner approach > >>would > >>>> > be to let drivers add custom syscon entries, which would then > >>require > >>>> > less custom plumbing. > >>>> > >>>> A syscon is convenient, sure, but it also bypasses any abstraction > >>>> layer we have everywhere else, which means that we'll have to > >>maintain > >>>> the register layout in each and every driver that uses it. > >>>> > >>>> So far, it's only be the GMAC, but it can also be others (the SRAM > >>>> controller comes to my mind), and then, if there's any difference in > >>>> the design in a future SoC, we'll have to maintain that in the GMAC > >>>> driver as well. > >>> > >>> I guess I forgot to say something, I'm fine with using a syscon we > >>> already have. > >>> > >>> I'm just questionning if merging any other driver using one is the > >>> right move. > >> > >>Right. So in this case, we are not actually going through the syscon > >>API. Rather we are exporting a regmap whose properties we actually > >>define. If it makes you more acceptable to it, we could map just > >>the GMAC register in the new regmap, and also have it named. This > >>is all plumbing within the kernel so the device tree stays the same. > > > > I think my driver has already restricted the write permission > > only to GMAC register. > > Correct, but it still maps the entire region out, which means the > consumer needs to know which offset to use. Maxime is saying this > is something that is troublesome to maintain. So my proposal was > to create a regmap with a base at the GMAC register offset. That > way, the consumer doesn't need to use an offset to access it. I guess this is something we can keep in mind if it gets out of control yse. Maxime
在 2018-04-03二的 11:50 +0200，Maxime Ripard写道： > On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: > > On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: > > > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard > > > <maxime.ripard@bootlin.com> wrote: > > > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: > > > > > From: Icenowy Zheng <icenowy@aosc.io> > > > > > > > > > > There's a GMAC configuration register, which exists on > > > > > A64/A83T/H3/H5 in > > > > > the syscon part, in the CCU of R40 SoC. > > > > > > > > > > Export a regmap of the CCU. > > > > > > > > > > Read access is not restricted to all registers, but only the > > > > > GMAC > > > > > register is allowed to be written. > > > > > > > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> > > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> > > > > > > > > Gah, this is crazy. I'm really starting to regret letting that > > > > syscon > > > > in in the first place... > > > > > > IMHO syscon is really a better fit. It's part of the glue layer > > > and > > > most other dwmac user platforms treat it as such and use a > > > syscon. > > > Plus the controls encompass delays (phase), inverters (polarity), > > > and even signal routing. It's not really just a group of clock > > > controls, > > > like what we poorly modeled for A20/A31. I think that was really > > > a > > > mistake. > > > > > > As I mentioned in the cover letter, a slightly saner approach > > > would > > > be to let drivers add custom syscon entries, which would then > > > require > > > less custom plumbing. > > > > A syscon is convenient, sure, but it also bypasses any abstraction > > layer we have everywhere else, which means that we'll have to > > maintain > > the register layout in each and every driver that uses it. > > > > So far, it's only be the GMAC, but it can also be others (the SRAM > > controller comes to my mind), and then, if there's any difference > > in > > the design in a future SoC, we'll have to maintain that in the GMAC > > driver as well. > > I guess I forgot to say something, I'm fine with using a syscon we > already have. As we finally need the SRAM controller on these new SoCs (for VE on all SoCs targeted by dwmac-sun8i, and for DE on A64), should we deprecate the syscon and all switch to device regmap (and let sunxi-dram driver to export a regmap)? (As in the A64 DE2 thread discussed, two device nodes shouldn't cover the same MMIO region.) In addition, there might be multiple registers in the syscon/sram zone that are needed (for example, if a version driver is written, it will need the "0x24 Version Register"; and GMAC needs "0x30 EMAC Clock Register"). How to deal with this if we export the syscon/sram zone as a generic device regmap? > > I'm just questionning if merging any other driver using one is the > right move. > > Maxime > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Wed, Apr 4, 2018 at 2:45 PM, Icenowy Zheng <icenowy@aosc.io> wrote: > 在 2018-04-03二的 11:50 +0200，Maxime Ripard写道： >> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote: >> > On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote: >> > > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard >> > > <maxime.ripard@bootlin.com> wrote: >> > > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote: >> > > > > From: Icenowy Zheng <icenowy@aosc.io> >> > > > > >> > > > > There's a GMAC configuration register, which exists on >> > > > > A64/A83T/H3/H5 in >> > > > > the syscon part, in the CCU of R40 SoC. >> > > > > >> > > > > Export a regmap of the CCU. >> > > > > >> > > > > Read access is not restricted to all registers, but only the >> > > > > GMAC >> > > > > register is allowed to be written. >> > > > > >> > > > > Signed-off-by: Icenowy Zheng <icenowy@aosc.io> >> > > > > Signed-off-by: Chen-Yu Tsai <wens@csie.org> >> > > > >> > > > Gah, this is crazy. I'm really starting to regret letting that >> > > > syscon >> > > > in in the first place... >> > > >> > > IMHO syscon is really a better fit. It's part of the glue layer >> > > and >> > > most other dwmac user platforms treat it as such and use a >> > > syscon. >> > > Plus the controls encompass delays (phase), inverters (polarity), >> > > and even signal routing. It's not really just a group of clock >> > > controls, >> > > like what we poorly modeled for A20/A31. I think that was really >> > > a >> > > mistake. >> > > >> > > As I mentioned in the cover letter, a slightly saner approach >> > > would >> > > be to let drivers add custom syscon entries, which would then >> > > require >> > > less custom plumbing. >> > >> > A syscon is convenient, sure, but it also bypasses any abstraction >> > layer we have everywhere else, which means that we'll have to >> > maintain >> > the register layout in each and every driver that uses it. >> > >> > So far, it's only be the GMAC, but it can also be others (the SRAM >> > controller comes to my mind), and then, if there's any difference >> > in >> > the design in a future SoC, we'll have to maintain that in the GMAC >> > driver as well. >> >> I guess I forgot to say something, I'm fine with using a syscon we >> already have. > > As we finally need the SRAM controller on these new SoCs (for VE on all > SoCs targeted by dwmac-sun8i, and for DE on A64), should we deprecate > the syscon and all switch to device regmap (and let sunxi-dram driver > to export a regmap)? (As in the A64 DE2 thread discussed, two device > nodes shouldn't cover the same MMIO region.) Sounds like a plan. > In addition, there might be multiple registers in the syscon/sram zone > that are needed (for example, if a version driver is written, it will > need the "0x24 Version Register"; and GMAC needs "0x30 EMAC Clock > Register"). How to deal with this if we export the syscon/sram zone as > a generic device regmap? You can have multiple regmaps for a device. And the API allows you to request them up by name. ChenYu >> >> I'm just questionning if merging any other driver using one is the >> right move. >> >> Maxime >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c index c3aa839a453d..54c7a6106206 100644 --- a/drivers/clk/sunxi-ng/ccu-sun8i-r40.c +++ b/drivers/clk/sunxi-ng/ccu-sun8i-r40.c @@ -1251,9 +1251,35 @@ static struct ccu_mux_nb sun8i_r40_cpu_nb = { .bypass_index = 1, /* index of 24 MHz oscillator */ }; +/* + * Add a regmap for the GMAC driver (dwmac-sun8i) to access the + * GMAC configuration register. + * Only this register is allowed to be written, in order to + * prevent overriding critical clock configuration. + */ + +#define SUN8I_R40_GMAC_CFG_REG 0x164 +static bool sun8i_r40_ccu_regmap_writeable_reg(struct device *dev, + unsigned int reg) +{ + if (reg == SUN8I_R40_GMAC_CFG_REG) + return true; + return false; +} + +static struct regmap_config sun8i_r40_ccu_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .max_register = 0x320, /* PLL_LOCK_CTRL_REG */ + + .writeable_reg = sun8i_r40_ccu_regmap_writeable_reg, +}; + static int sun8i_r40_ccu_probe(struct platform_device *pdev) { struct resource *res; + struct regmap *regmap; void __iomem *reg; u32 val; int ret; @@ -1278,6 +1304,11 @@ static int sun8i_r40_ccu_probe(struct platform_device *pdev) val &= ~GENMASK(25, 20); writel(val, reg + SUN8I_R40_USB_CLK_REG); + regmap = devm_regmap_init_mmio(&pdev->dev, reg, + &sun8i_r40_ccu_regmap_config); + if (IS_ERR(regmap)) + return PTR_ERR(regmap); + ret = sunxi_ccu_probe(pdev->dev.of_node, reg, &sun8i_r40_ccu_desc); if (ret) return ret;