Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-17 Thread Icenowy Zheng 


于 2018年4月17日 GMT+08:00 下午7:59:38, Chen-Yu Tsai  写到:
>On Tue, Apr 17, 2018 at 7:52 PM, Maxime Ripard
> 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
>>>  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 
>wrote:
>>> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard
> 写到:
>>> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>>> >> >>> From: Chen-Yu Tsai 
>>> >> >>>
>>> >> >>> 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 
>>> >> >>> [Icenowy: change to use regmaps with single register, change
>commit
>>> >> >>>  message]
>>> >> >>> Signed-off-by: Icenowy Zheng 
>>> >> >>> ---
>>> >> >>>  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-ker...@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-17 Thread Chen-Yu Tsai 
On Tue, Apr 17, 2018 at 7:52 PM, Maxime Ripard
 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
>>  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  wrote:
>> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
>> >> >  写到:
>> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>> >> >>> From: Chen-Yu Tsai 
>> >> >>>
>> >> >>> 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 
>> >> >>> [Icenowy: change to use regmaps with single register, change commit
>> >> >>>  message]
>> >> >>> Signed-off-by: Icenowy Zheng 
>> >> >>> ---
>> >> >>>  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

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-17 Thread Maxime Ripard 
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
>  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  wrote:
> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
> >> >  写到:
> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> >> >>> From: Chen-Yu Tsai 
> >> >>>
> >> >>> 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 
> >> >>> [Icenowy: change to use regmaps with single register, change commit
> >> >>>  message]
> >> >>> Signed-off-by: Icenowy Zheng 
> >> >>> ---
> >> >>>  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

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-16 Thread Chen-Yu Tsai 
On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard
 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  wrote:
>> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
>> >  写到:
>> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>> >>> From: Chen-Yu Tsai 
>> >>>
>> >>> 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 
>> >>> [Icenowy: change to use regmaps with single register, change commit
>> >>>  message]
>> >>> Signed-off-by: Icenowy Zheng 
>> >>> ---
>> >>>  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

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-16 Thread Icenowy Zheng 


于 2018年4月16日 GMT+08:00 下午10:31:30, Maxime Ripard  写到:
>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 
>wrote:
>> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard
> 写到:
>> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>> >>> From: Chen-Yu Tsai 
>> >>>
>> >>> 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 
>> >>> [Icenowy: change to use regmaps with single register, change
>commit
>> >>>  message]
>> >>> Signed-off-by: Icenowy Zheng 
>> >>> ---
>> >>>  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

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-16 Thread Maxime Ripard 
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  wrote:
> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
> >  写到:
> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> >>> From: Chen-Yu Tsai 
> >>>
> >>> 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 
> >>> [Icenowy: change to use regmaps with single register, change commit
> >>>  message]
> >>> Signed-off-by: Icenowy Zheng 
> >>> ---
> >>>  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

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-13 Thread kbuild test robot 
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-12 Thread Chen-Yu Tsai 
On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng  wrote:
>
>
> 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard  
> 写到:
>>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>>> From: Chen-Yu Tsai 
>>>
>>> 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 
>>> [Icenowy: change to use regmaps with single register, change commit
>>>  message]
>>> Signed-off-by: Icenowy Zheng 
>>> ---
>>>  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

Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-12 Thread Icenowy Zheng 


于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard  写到:
>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
>> From: Chen-Yu Tsai 
>> 
>> 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 
>> [Icenowy: change to use regmaps with single register, change commit
>>  message]
>> Signed-off-by: Icenowy Zheng 
>> ---
>>  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

Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

2018-04-12 Thread Maxime Ripard 
On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> From: Chen-Yu Tsai 
> 
> 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 
> [Icenowy: change to use regmaps with single register, change commit
>  message]
> Signed-off-by: Icenowy Zheng 
> ---
>  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

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature