| Message ID | 20190624040212.3726-10-anup.patel@wdc.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | Update SiFive Unleashed Drivers | expand |
On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: > > We should depend on __LITTLE_ENDIAN pre-defined compiler macro for > little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN > macro. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > drivers/net/macb.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
On 6/24/19 8:03 AM, Bin Meng wrote: > On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: >> We should depend on __LITTLE_ENDIAN pre-defined compiler macro for >> little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN >> macro. >> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> --- >> drivers/net/macb.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Hi. I don't like this approach, each platform should configure it's endianess, this is stated in README in root folder. relying on a specific GCC preprocessor extension is limiting us only to use GCC. The RISCV issue with MACB can be easily resolved by defining the CONFIG_SYS_LITTLE_ENDIAN config. Thanks, Ramon.
Hi Ramon, On Mon, Jun 24, 2019 at 8:22 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > On 6/24/19 8:03 AM, Bin Meng wrote: > > On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: > > We should depend on __LITTLE_ENDIAN pre-defined compiler macro for > little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN > macro. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > --- > drivers/net/macb.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > Hi. > I don't like this approach, each platform should configure it's > endianess, this is stated in README in root folder. > relying on a specific GCC preprocessor extension is limiting us only to use GCC. > The RISCV issue with MACB can be easily resolved by defining the > CONFIG_SYS_LITTLE_ENDIAN config. OK, but a system wide CONFIG_SYS_LITTLE_ENDIAN may bring side effects to other drivers, as not all devices are using the same endianness even in the same system. Maybe we can do something by parsing some property in device tree? Regards, Bin
On 6/24/19 3:32 PM, Bin Meng wrote: > Hi Ramon, > > On Mon, Jun 24, 2019 at 8:22 PM Ramon Fried <rfried.dev@gmail.com> wrote: >> >> On 6/24/19 8:03 AM, Bin Meng wrote: >> >> On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: >> >> We should depend on __LITTLE_ENDIAN pre-defined compiler macro for >> little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN >> macro. >> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> --- >> drivers/net/macb.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> >> Hi. >> I don't like this approach, each platform should configure it's >> endianess, this is stated in README in root folder. >> relying on a specific GCC preprocessor extension is limiting us only to use GCC. >> The RISCV issue with MACB can be easily resolved by defining the >> CONFIG_SYS_LITTLE_ENDIAN config. > OK, but a system wide CONFIG_SYS_LITTLE_ENDIAN may bring side effects > to other drivers, as not all devices are using the same endianness > even in the same system. Maybe we can do something by parsing some > property in device tree? > > Regards, > Bin Hey Bin I grep'ed for all instances of CONFIG_SYS_LITTLE_ENDIAN and I don't see any place where something might brake. can you elaborate ? Thanks, Ramon.
On Mon, Jun 24, 2019 at 2:51 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > On 6/24/19 3:32 PM, Bin Meng wrote: > > Hi Ramon, > > > > On Mon, Jun 24, 2019 at 8:22 PM Ramon Fried <rfried.dev@gmail.com> wrote: > >> > >> On 6/24/19 8:03 AM, Bin Meng wrote: > >> > >> On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: > >> > >> We should depend on __LITTLE_ENDIAN pre-defined compiler macro for > >> little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN > >> macro. > >> > >> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> --- > >> drivers/net/macb.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > >> > >> Hi. > >> I don't like this approach, each platform should configure it's > >> endianess, this is stated in README in root folder. > >> relying on a specific GCC preprocessor extension is limiting us only to use GCC. > >> The RISCV issue with MACB can be easily resolved by defining the > >> CONFIG_SYS_LITTLE_ENDIAN config. > > OK, but a system wide CONFIG_SYS_LITTLE_ENDIAN may bring side effects > > to other drivers, as not all devices are using the same endianness > > even in the same system. Maybe we can do something by parsing some > > property in device tree? > > > > Regards, > > Bin > > Hey Bin > > I grep'ed for all instances of CONFIG_SYS_LITTLE_ENDIAN and I don't see any place > where something might brake. can you elaborate ? > can't you simply remove the non-DM part of the driver and add a device-tree property for the endianess? If that's not possible short-term, than maybe add a Kconfig symbol like this: config MACB_BIG_ENDIAN bool "...." depends on MACB default y if SYS_BIG_ENDIAN default n this way it's automatically sync'ed with CONFIG_SYS_LITTLE_ENDIAN/CONFIG_SYS_BIG_ENDIAN, but a board config could still override this setting.
On 6/24/19 4:02 PM, Daniel Schwierzeck wrote: > can't you simply remove the non-DM part of the driver and add a > device-tree property for the endianess? > > If that's not possible short-term, than maybe add a Kconfig symbol like this: > > config MACB_BIG_ENDIAN > bool "...." > depends on MACB > default y if SYS_BIG_ENDIAN > default n > > this way it's automatically sync'ed with > CONFIG_SYS_LITTLE_ENDIAN/CONFIG_SYS_BIG_ENDIAN, but a board config > could still override this setting. I can even do better, Linux for instance automatically detects the endianess for the CPU by writing to a scratch register on MACB HW and read it back. But I don't understand the need for a specific board to define anything different than the SYS_ENDIANES for this driver. Thanks, Ramon.
Hi Ramon, On Mon, Jun 24, 2019 at 8:51 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > On 6/24/19 3:32 PM, Bin Meng wrote: > > Hi Ramon, > > > > On Mon, Jun 24, 2019 at 8:22 PM Ramon Fried <rfried.dev@gmail.com> wrote: > >> > >> On 6/24/19 8:03 AM, Bin Meng wrote: > >> > >> On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: > >> > >> We should depend on __LITTLE_ENDIAN pre-defined compiler macro for > >> little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN > >> macro. > >> > >> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> --- > >> drivers/net/macb.c | 7 ++++--- > >> 1 file changed, 4 insertions(+), 3 deletions(-) > >> > >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > >> > >> Hi. > >> I don't like this approach, each platform should configure it's > >> endianess, this is stated in README in root folder. > >> relying on a specific GCC preprocessor extension is limiting us only to use GCC. > >> The RISCV issue with MACB can be easily resolved by defining the > >> CONFIG_SYS_LITTLE_ENDIAN config. > > OK, but a system wide CONFIG_SYS_LITTLE_ENDIAN may bring side effects > > to other drivers, as not all devices are using the same endianness > > even in the same system. Maybe we can do something by parsing some > > property in device tree? > > > > Regards, > > Bin > > Hey Bin > > I grep'ed for all instances of CONFIG_SYS_LITTLE_ENDIAN and I don't see any place > where something might brake. can you elaborate ? I mean this system wide CONFIG_SYS_LITTLE_ENDIAN is easy to break since it cannot represent all devices, although I did not check all instances currently in U-Boot. Maybe it's OK for now for the SiFive board. But this option is not better than the pure compiler flag either. So I was proposing using some properties in DT. Does that help? Regards, Bin
On 6/24/19 4:12 PM, Bin Meng wrote: > Hi Ramon, > > On Mon, Jun 24, 2019 at 8:51 PM Ramon Fried <rfried.dev@gmail.com> wrote: >> >> On 6/24/19 3:32 PM, Bin Meng wrote: >>> Hi Ramon, >>> >>> On Mon, Jun 24, 2019 at 8:22 PM Ramon Fried <rfried.dev@gmail.com> wrote: >>>> On 6/24/19 8:03 AM, Bin Meng wrote: >>>> >>>> On Mon, Jun 24, 2019 at 12:03 PM Anup Patel <Anup.Patel@wdc.com> wrote: >>>> >>>> We should depend on __LITTLE_ENDIAN pre-defined compiler macro for >>>> little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN >>>> macro. >>>> >>>> Signed-off-by: Anup Patel <anup.patel@wdc.com> >>>> --- >>>> drivers/net/macb.c | 7 ++++--- >>>> 1 file changed, 4 insertions(+), 3 deletions(-) >>>> >>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >>>> >>>> Hi. >>>> I don't like this approach, each platform should configure it's >>>> endianess, this is stated in README in root folder. >>>> relying on a specific GCC preprocessor extension is limiting us only to use GCC. >>>> The RISCV issue with MACB can be easily resolved by defining the >>>> CONFIG_SYS_LITTLE_ENDIAN config. >>> OK, but a system wide CONFIG_SYS_LITTLE_ENDIAN may bring side effects >>> to other drivers, as not all devices are using the same endianness >>> even in the same system. Maybe we can do something by parsing some >>> property in device tree? >>> >>> Regards, >>> Bin >> Hey Bin >> >> I grep'ed for all instances of CONFIG_SYS_LITTLE_ENDIAN and I don't see any place >> where something might brake. can you elaborate ? > I mean this system wide CONFIG_SYS_LITTLE_ENDIAN is easy to break > since it cannot represent all devices, although I did not check all > instances currently in U-Boot. Maybe it's OK for now for the SiFive > board. But this option is not better than the pure compiler flag > either. So I was proposing using some properties in DT. Does that > help? Not all devices work using DT in macb, so it won't work for them. :( Thanks, Ramon. > > Regards, > Bin
On Mon, Jun 24, 2019 at 6:37 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > > On 6/24/19 4:02 PM, Daniel Schwierzeck wrote: > > can't you simply remove the non-DM part of the driver and add a > > device-tree property for the endianess? > > > > If that's not possible short-term, than maybe add a Kconfig symbol like this: > > > > config MACB_BIG_ENDIAN > > bool "...." > > depends on MACB > > default y if SYS_BIG_ENDIAN > > default n > > > > this way it's automatically sync'ed with > > CONFIG_SYS_LITTLE_ENDIAN/CONFIG_SYS_BIG_ENDIAN, but a board config > > could still override this setting. > > I can even do better, Linux for instance automatically detects the endianess for the CPU > by writing to a scratch register on MACB HW and read it back. > > But I don't understand the need for a specific board to define anything different than the SYS_ENDIANES for this driver. Yap, it's very easy to detect endianess at runtime. I will update this patch accordingly. Regards, Anup
diff --git a/drivers/net/macb.c b/drivers/net/macb.c index 727cb0bc49..1cfdfd817a 100644 --- a/drivers/net/macb.c +++ b/drivers/net/macb.c @@ -740,10 +740,10 @@ static void gmac_configure_dma(struct macb_device *macb) dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L); dmacfg &= ~GEM_BIT(ENDIA_PKT); -#ifdef CONFIG_SYS_LITTLE_ENDIAN - dmacfg &= ~GEM_BIT(ENDIA_DESC); +#ifdef __LITTLE_ENDIAN + dmacfg &= ~GEM_BIT(ENDIA_DESC); #else - dmacfg |= GEM_BIT(ENDIA_DESC); /* CPU in big endian */ + dmacfg |= GEM_BIT(ENDIA_DESC); /* CPU in big endian */ #endif dmacfg &= ~GEM_BIT(ADDR64); @@ -1306,6 +1306,7 @@ static const struct macb_config sama5d4_config = { }; static const struct macb_config sifive_config = { + .dma_burst_length = 16, .probe = macb_sifive_probe, .set_tx_clk = macb_sifive_set_tx_clk, };
We should depend on __LITTLE_ENDIAN pre-defined compiler macro for little-endian system instead of U-Boot specific CONFIG_SYS_LITTLE_ENDIAN macro. Signed-off-by: Anup Patel <anup.patel@wdc.com> --- drivers/net/macb.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)