* [PATCH 0/2] leds: bcm63x8: improve read and write functions @ 2021-02-23 8:17 Álvaro Fernández Rojas 2021-02-23 8:17 ` [PATCH 1/2] leds: bcm6328: improve write and read functions Álvaro Fernández Rojas 2021-02-23 8:17 ` [PATCH 2/2] leds: bcm6358: " Álvaro Fernández Rojas 0 siblings, 2 replies; 11+ messages in thread From: Álvaro Fernández Rojas @ 2021-02-23 8:17 UTC (permalink / raw) To: f.fainelli, Pavel Machek, Dan Murphy, linux-leds, linux-kernel Cc: Álvaro Fernández Rojas This code is proven to work in BMIPS BE/LE and ARM BE/LE. See bcm2835-rng and bcmgenet.c: https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60 https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88 Álvaro Fernández Rojas (2): leds: bcm6328: improve write and read functions leds: bcm6358: improve write and read functions drivers/leds/leds-bcm6328.c | 25 +++++++++++++------------ drivers/leds/leds-bcm6358.c | 25 +++++++++++++------------ 2 files changed, 26 insertions(+), 24 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 8:17 [PATCH 0/2] leds: bcm63x8: improve read and write functions Álvaro Fernández Rojas @ 2021-02-23 8:17 ` Álvaro Fernández Rojas 2021-02-23 8:34 ` Pavel Machek 2021-02-23 8:17 ` [PATCH 2/2] leds: bcm6358: " Álvaro Fernández Rojas 1 sibling, 1 reply; 11+ messages in thread From: Álvaro Fernández Rojas @ 2021-02-23 8:17 UTC (permalink / raw) To: f.fainelli, Pavel Machek, Dan Murphy, linux-leds, linux-kernel Cc: Álvaro Fernández Rojas This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng and bcmgenet drivers. Both should also be inline functions. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- drivers/leds/leds-bcm6328.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/leds/leds-bcm6328.c b/drivers/leds/leds-bcm6328.c index 226d17d253ed..112c9c858432 100644 --- a/drivers/leds/leds-bcm6328.c +++ b/drivers/leds/leds-bcm6328.c @@ -75,22 +75,23 @@ struct bcm6328_led { bool active_low; }; -static void bcm6328_led_write(void __iomem *reg, unsigned long data) +static inline void bcm6328_led_write(void __iomem *reg, unsigned long data) { -#ifdef CONFIG_CPU_BIG_ENDIAN - iowrite32be(data, reg); -#else - writel(data, reg); -#endif + /* MIPS chips strapped for BE will automagically configure the + * peripheral registers for CPU-native byte order. + */ + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) + __raw_writel(data, reg); + else + writel_relaxed(data, reg); } -static unsigned long bcm6328_led_read(void __iomem *reg) +static inline unsigned long bcm6328_led_read(void __iomem *reg) { -#ifdef CONFIG_CPU_BIG_ENDIAN - return ioread32be(reg); -#else - return readl(reg); -#endif + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) + return __raw_readl(reg); + else + return readl_relaxed(reg); } /** -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 8:17 ` [PATCH 1/2] leds: bcm6328: improve write and read functions Álvaro Fernández Rojas @ 2021-02-23 8:34 ` Pavel Machek 2021-02-23 8:42 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 11+ messages in thread From: Pavel Machek @ 2021-02-23 8:34 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: f.fainelli, Dan Murphy, linux-leds, linux-kernel [-- Attachment #1: Type: text/plain, Size: 922 bytes --] On Tue 2021-02-23 09:17:31, Álvaro Fernández Rojas wrote: > This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng > and bcmgenet drivers. > Both should also be inline functions. > -#ifdef CONFIG_CPU_BIG_ENDIAN > - iowrite32be(data, reg); > -#else > - writel(data, reg); > -#endif > + /* MIPS chips strapped for BE will automagically configure the > + * peripheral registers for CPU-native byte order. > + */ Bad comment style. > + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) > + __raw_writel(data, reg); > + else > + writel_relaxed(data, reg); > } Code does not match comment (still need to do conversion on non-MIPS?), and it certainly should not be here (do all mipsen behave like that?!), and it really should not be converting to _relaxed at the same time. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 8:34 ` Pavel Machek @ 2021-02-23 8:42 ` Álvaro Fernández Rojas 2021-02-23 8:58 ` Pavel Machek 0 siblings, 1 reply; 11+ messages in thread From: Álvaro Fernández Rojas @ 2021-02-23 8:42 UTC (permalink / raw) To: Pavel Machek; +Cc: Florian Fainelli, Dan Murphy, linux-leds, linux-kernel Hi Pavel, > El 23 feb 2021, a las 9:34, Pavel Machek <pavel@ucw.cz> escribió: > > On Tue 2021-02-23 09:17:31, Álvaro Fernández Rojas wrote: >> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng >> and bcmgenet drivers. >> Both should also be inline functions. > > > >> -#ifdef CONFIG_CPU_BIG_ENDIAN >> - iowrite32be(data, reg); >> -#else >> - writel(data, reg); >> -#endif >> + /* MIPS chips strapped for BE will automagically configure the >> + * peripheral registers for CPU-native byte order. >> + */ > > Bad comment style. I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet… https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60 https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88 > >> + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) >> + __raw_writel(data, reg); >> + else >> + writel_relaxed(data, reg); >> } > > Code does not match comment (still need to do conversion on > non-MIPS?), and it certainly should not be here (do all mipsen behave > like that?!), and it really should not be converting to _relaxed at > the same time. I think it's because non-MIPS BE exposes that as little endian, but Florian can probably help us with that… > > Best regards, > Pavel > -- > http://www.livejournal.com/~pavelmachek Best regards, Álvaro. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 8:42 ` Álvaro Fernández Rojas @ 2021-02-23 8:58 ` Pavel Machek 2021-02-23 9:05 ` Álvaro Fernández Rojas 0 siblings, 1 reply; 11+ messages in thread From: Pavel Machek @ 2021-02-23 8:58 UTC (permalink / raw) To: Álvaro Fernández Rojas Cc: Florian Fainelli, Dan Murphy, linux-leds, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1020 bytes --] Hi! > >> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng > >> and bcmgenet drivers. > >> Both should also be inline functions. > > > > > > > >> -#ifdef CONFIG_CPU_BIG_ENDIAN > >> - iowrite32be(data, reg); > >> -#else > >> - writel(data, reg); > >> -#endif > >> + /* MIPS chips strapped for BE will automagically configure the > >> + * peripheral registers for CPU-native byte order. > >> + */ > > > > Bad comment style. > > I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet… > https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60 > https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88 > Yeah, but ideally you should not be copying comments; there should be one central place which does it and does it right. Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 8:58 ` Pavel Machek @ 2021-02-23 9:05 ` Álvaro Fernández Rojas 2021-02-23 17:00 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Álvaro Fernández Rojas @ 2021-02-23 9:05 UTC (permalink / raw) To: Pavel Machek; +Cc: Florian Fainelli, Dan Murphy, linux-leds, linux-kernel > El 23 feb 2021, a las 9:58, Pavel Machek <pavel@ucw.cz> escribió: > > Hi! > >>>> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng >>>> and bcmgenet drivers. >>>> Both should also be inline functions. >>> >>> >>> >>>> -#ifdef CONFIG_CPU_BIG_ENDIAN >>>> - iowrite32be(data, reg); >>>> -#else >>>> - writel(data, reg); >>>> -#endif >>>> + /* MIPS chips strapped for BE will automagically configure the >>>> + * peripheral registers for CPU-native byte order. >>>> + */ >>> >>> Bad comment style. >> >> I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet… >> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60 >> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88 >> > > Yeah, but ideally you should not be copying comments; there should be > one central place which does it and does it right. I’m open to suggestions :). Which central place would be a good place for you? > > Pavel > > -- > http://www.livejournal.com/~pavelmachek ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 9:05 ` Álvaro Fernández Rojas @ 2021-02-23 17:00 ` Florian Fainelli 2021-02-24 17:36 ` Pavel Machek 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2021-02-23 17:00 UTC (permalink / raw) To: Álvaro Fernández Rojas, Pavel Machek Cc: Dan Murphy, linux-leds, linux-kernel On 2/23/2021 1:05 AM, Álvaro Fernández Rojas wrote: > > >> El 23 feb 2021, a las 9:58, Pavel Machek <pavel@ucw.cz> escribió: >> >> Hi! >> >>>>> This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng >>>>> and bcmgenet drivers. >>>>> Both should also be inline functions. >>>> >>>> >>>> >>>>> -#ifdef CONFIG_CPU_BIG_ENDIAN >>>>> - iowrite32be(data, reg); >>>>> -#else >>>>> - writel(data, reg); >>>>> -#endif >>>>> + /* MIPS chips strapped for BE will automagically configure the >>>>> + * peripheral registers for CPU-native byte order. >>>>> + */ >>>> >>>> Bad comment style. >>> >>> I just wanted to copy the same comment as the one in bcm2835-rng and bcmgenet… >>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/char/hw_random/bcm2835-rng.c#L42-L60 >>> https://github.com/torvalds/linux/blob/3b9cdafb5358eb9f3790de2f728f765fef100731/drivers/net/ethernet/broadcom/genet/bcmgenet.c#L71-L88 >>> >> >> Yeah, but ideally you should not be copying comments; there should be >> one central place which does it and does it right. > > I’m open to suggestions :). > Which central place would be a good place for you? I did consider creating an include/linux/brcm/brcm_io.h header or something like that but I am really not sure what the benefit would be. As far as using _relaxed() this is absolutely correct because the bus logic that connects the CPU to its on-chip registers is non re-ordering non posted. That is true on the MIPS BE/LE and ARM when configured in LE or BE. We need the swapping for ARM because when running in ARM BE32, the data is going to be in the host CPU endian, but the register bus is hard wired to little endian. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-23 17:00 ` Florian Fainelli @ 2021-02-24 17:36 ` Pavel Machek 2021-02-24 17:45 ` Florian Fainelli 0 siblings, 1 reply; 11+ messages in thread From: Pavel Machek @ 2021-02-24 17:36 UTC (permalink / raw) To: Florian Fainelli Cc: Álvaro Fernández Rojas, Dan Murphy, linux-leds, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1411 bytes --] Hi! > >> Yeah, but ideally you should not be copying comments; there should be > >> one central place which does it and does it right. > > > > I’m open to suggestions :). > > Which central place would be a good place for you? > > I did consider creating an include/linux/brcm/brcm_io.h header or > something like that but I am really not sure what the benefit would > be. Less code duplication? It is immediately clear that driver including this is specific for brcm SoCs and would not try to work somewhere else? > As far as using _relaxed() this is absolutely correct because the bus > logic that connects the CPU to its on-chip registers is non re-ordering > non posted. That is true on the MIPS BE/LE and ARM when configured in LE > or BE. If that's right on particular SoC, then _relaxed and normal versions should be same; drivers still need to use normal versions, because they may be running on different SoC...? > We need the swapping for ARM because when running in ARM BE32, the data > is going to be in the host CPU endian, but the register bus is hard > wired to little endian. Yeah I see you need to do some byteswapping. But I'm pretty sure not all MIPS BE boxes do the magic swapping, right? And drivers/leds is not a place where you encode knowledge about SoC byte swapping. Best regards, Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 181 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-24 17:36 ` Pavel Machek @ 2021-02-24 17:45 ` Florian Fainelli 2021-02-24 21:45 ` Pavel Machek 0 siblings, 1 reply; 11+ messages in thread From: Florian Fainelli @ 2021-02-24 17:45 UTC (permalink / raw) To: Pavel Machek; +Cc: Álvaro Fernández Rojas, linux-leds, linux-kernel On 2/24/2021 9:36 AM, Pavel Machek wrote: > Hi! > >>>> Yeah, but ideally you should not be copying comments; there should be >>>> one central place which does it and does it right. >>> >>> I’m open to suggestions :). >>> Which central place would be a good place for you? >> >> I did consider creating an include/linux/brcm/brcm_io.h header or >> something like that but I am really not sure what the benefit would >> be. > > Less code duplication? It is immediately clear that driver including > this is specific for brcm SoCs and would not try to work somewhere else? Yes maybe, there still does not feel like this deserves a shared header, but as long as the generated code is the same, why not. > >> As far as using _relaxed() this is absolutely correct because the bus >> logic that connects the CPU to its on-chip registers is non re-ordering >> non posted. That is true on the MIPS BE/LE and ARM when configured in LE >> or BE. > > If that's right on particular SoC, then _relaxed and normal versions > should be same; drivers still need to use normal versions, because > they may be running on different SoC...? readl() includes barriers and read_relaxed() does not, hence the difference in the name. There is no need to pay the price of a barrier when a) the bus architecture guarantees non re-ordering and posting and that statement is true on all the SoCs where these peripherals are used, and b) you have worked on fine tuning your drivers to get the most performance out of them. > >> We need the swapping for ARM because when running in ARM BE32, the data >> is going to be in the host CPU endian, but the register bus is hard >> wired to little endian. > > Yeah I see you need to do some byteswapping. But I'm pretty sure not > all MIPS BE boxes do the magic swapping, right? And drivers/leds is > not a place where you encode knowledge about SoC byte swapping. The Broadcom MIPS CPUs (we have/had an architectural license) can be strapped for BE or LE, and when that happens the bridge that connects to the registers follows the CPU's endian, which is why __raw_{read,write}l is appropriate for these specific peripherals. Given these peripherals can only be used on CPUs/SoCs made by Broadcom, any argument about portability to other SoCs is moot. -- Florian ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] leds: bcm6328: improve write and read functions 2021-02-24 17:45 ` Florian Fainelli @ 2021-02-24 21:45 ` Pavel Machek 0 siblings, 0 replies; 11+ messages in thread From: Pavel Machek @ 2021-02-24 21:45 UTC (permalink / raw) To: Florian Fainelli Cc: Álvaro Fernández Rojas, linux-leds, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1636 bytes --] Hi! > > Less code duplication? It is immediately clear that driver including > > this is specific for brcm SoCs and would not try to work somewhere else? > > Yes maybe, there still does not feel like this deserves a shared header, > but as long as the generated code is the same, why not. Ok, it seems patch is not needed at all, after all? > > > >> As far as using _relaxed() this is absolutely correct because the bus > >> logic that connects the CPU to its on-chip registers is non re-ordering > >> non posted. That is true on the MIPS BE/LE and ARM when configured in LE > >> or BE. > > > > If that's right on particular SoC, then _relaxed and normal versions > > should be same; drivers still need to use normal versions, because > > they may be running on different SoC...? > > readl() includes barriers and read_relaxed() does not, hence the > difference in the name. There is no need to pay the price of a barrier > when a) the bus architecture guarantees non re-ordering and posting and > that statement is true on all the SoCs where these peripherals are used, > and b) you have worked on fine tuning your drivers to get the most > performance out of them. Exactly. When bus architecture guarantees ... readl and read_relaxed can be the same. That knowledge should be in architecture code, not in drivers. (But it does not matter much when the drivers are architecture-specific). > Given these peripherals can only be used on CPUs/SoCs made by Broadcom, > any argument about portability to other SoCs is moot. Ok. Pavel -- http://www.livejournal.com/~pavelmachek [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 195 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] leds: bcm6358: improve write and read functions 2021-02-23 8:17 [PATCH 0/2] leds: bcm63x8: improve read and write functions Álvaro Fernández Rojas 2021-02-23 8:17 ` [PATCH 1/2] leds: bcm6328: improve write and read functions Álvaro Fernández Rojas @ 2021-02-23 8:17 ` Álvaro Fernández Rojas 1 sibling, 0 replies; 11+ messages in thread From: Álvaro Fernández Rojas @ 2021-02-23 8:17 UTC (permalink / raw) To: f.fainelli, Pavel Machek, Dan Murphy, linux-leds, linux-kernel Cc: Álvaro Fernández Rojas This is proven to work in BMIPS BE/LE and ARM BE/LE, as used in bcm2835-rng and bcmgenet drivers. Both should also be inline functions. Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com> --- drivers/leds/leds-bcm6358.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/leds/leds-bcm6358.c b/drivers/leds/leds-bcm6358.c index 9d2e487fa08a..775b8c8b562a 100644 --- a/drivers/leds/leds-bcm6358.c +++ b/drivers/leds/leds-bcm6358.c @@ -43,22 +43,23 @@ struct bcm6358_led { bool active_low; }; -static void bcm6358_led_write(void __iomem *reg, unsigned long data) +static inline void bcm6358_led_write(void __iomem *reg, unsigned long data) { -#ifdef CONFIG_CPU_BIG_ENDIAN - iowrite32be(data, reg); -#else - writel(data, reg); -#endif + /* MIPS chips strapped for BE will automagically configure the + * peripheral registers for CPU-native byte order. + */ + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) + __raw_writel(data, reg); + else + writel_relaxed(data, reg); } -static unsigned long bcm6358_led_read(void __iomem *reg) +static inline unsigned long bcm6358_led_read(void __iomem *reg) { -#ifdef CONFIG_CPU_BIG_ENDIAN - return ioread32be(reg); -#else - return readl(reg); -#endif + if (IS_ENABLED(CONFIG_MIPS) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) + return __raw_readl(reg); + else + return readl_relaxed(reg); } static unsigned long bcm6358_led_busy(void __iomem *mem) -- 2.20.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-24 21:46 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-02-23 8:17 [PATCH 0/2] leds: bcm63x8: improve read and write functions Álvaro Fernández Rojas 2021-02-23 8:17 ` [PATCH 1/2] leds: bcm6328: improve write and read functions Álvaro Fernández Rojas 2021-02-23 8:34 ` Pavel Machek 2021-02-23 8:42 ` Álvaro Fernández Rojas 2021-02-23 8:58 ` Pavel Machek 2021-02-23 9:05 ` Álvaro Fernández Rojas 2021-02-23 17:00 ` Florian Fainelli 2021-02-24 17:36 ` Pavel Machek 2021-02-24 17:45 ` Florian Fainelli 2021-02-24 21:45 ` Pavel Machek 2021-02-23 8:17 ` [PATCH 2/2] leds: bcm6358: " Álvaro Fernández Rojas
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.